Skip to content

MINOR: Remove unused StreamsGraphNode#repartitionRequired#6227

Merged
bbejeck merged 1 commit intoapache:trunkfrom
dongjinleekr:cleanup/201902
Feb 26, 2019
Merged

MINOR: Remove unused StreamsGraphNode#repartitionRequired#6227
bbejeck merged 1 commit intoapache:trunkfrom
dongjinleekr:cleanup/201902

Conversation

@dongjinleekr
Copy link
Copy Markdown
Contributor

@dongjinleekr dongjinleekr commented Feb 2, 2019

I found this defect while inspecting KAFKA-7293: Merge followed by groupByKey/join might violate co-partitioning; This flag is never used now. Instead, KStreamImpl#repartitionRequired is now covering its functionality.

cc/ @mjsax

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mjsax mjsax added the streams label Feb 14, 2019
mjsax
mjsax approved these changes Feb 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @dongjinleekr! Had a quick look an LGTM (from what I can tell). Call for second review @bbejeck

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Feb 21, 2019

Ping @bbejeck for second review.

bbejeck
bbejeck approved these changes Feb 21, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @dongjinleekr, LGTM.

Waiting for Jenkins to finish for merging.

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 21, 2019

retest this please

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 22, 2019

Java 8 failure kafka.api.AdminClientIntegrationTest.testMinimumRequestTimeouts
Java 11 failure kafka.api.SaslSslAdminClientIntegrationTest.testMinimumRequestTimeouts

retest this please

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 22, 2019 •
edited
Loading

Java 8 passed, Java 11 failed

org.apache.kafka.common.network.SslTransportLayerTest.testUnsupportedCiphers
kafka.api.ClientIdQuotaTest.testThrottledProducerConsumer
kafka.api.PlaintextConsumerTest.testMaxPollRecords

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 22, 2019

retest this please

@dongjinleekr
Copy link
Copy Markdown
Contributor Author

dongjinleekr commented Feb 22, 2019

Rebased against the latest trunk.

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 22, 2019

Java 8 failed kafka.api.AdminClientIntegrationTest.testMinimumRequestTimeouts , Java 11 passed this time.

retest this please

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 23, 2019

Java 8 passed, Java 11 failed (results already gone)

retest this please

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 25, 2019

Java 8 passed, Java 11 failed kafka.cluster.PartitionTest.testDelayedFetchAfterAppendRecords

retest this please

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 25, 2019

retest this please

@bbejeck bbejeck merged commit dc91ce5 into apache:trunk Feb 26, 2019
@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 26, 2019

merged #6227 to trunk

@bbejeck
Copy link
Copy Markdown
Member

bbejeck commented Feb 26, 2019

Thanks @dongjinleekr for the contribution!

jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
@stanislavkozlovski
Merge remote-tracking branch 'AK/trunk' into cos-trunk
d516eac 
* AK/trunk: (36 commits)
  KAFKA-7962: Avoid NPE for StickyAssignor (apache#6308)
  Address flakiness of CustomQuotaCallbackTest#testCustomQuotaCallback (apache#6330)
  KAFKA-7918: Inline generic parameters Pt. II: RocksDB Bytes Store and Memory LRU Caches (apache#6327)
  MINOR: fix parameter naming (apache#6316)
  KAFKA-7956 In ShutdownableThread, immediately complete the shutdown if the thread has not been started (apache#6218)
  MINOR: Refactor replica log dir fetching for improved logging (apache#6313)
  [TRIVIAL] Remove unused StreamsGraphNode#repartitionRequired (apache#6227)
  MINOR: Increase produce timeout to 120 seconds (apache#6326)
  KAFKA-7918: Inline generic parameters Pt. I: in-memory key-value store (apache#6293)
  MINOR: Fix line break issue in upgrade notes (apache#6320)
  KAFKA-7972: Use automatic RPC generation in SaslHandshake
  MINOR: Enable capture of full stack trace in StreamTask#process (apache#6310)
  KAFKA-7938: Fix test flakiness in DeleteConsumerGroupsTest (apache#6312)
  KAFKA-7937: Fix Flaky Test ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup (apache#6311)
  MINOR: Update docs to say 2.2 (apache#6315)
  KAFKA-7672 : force write checkpoint during StreamTask #suspend (apache#6115)
  KAFKA-7961; Ignore assignment for un-subscribed partitions (apache#6304)
  KAFKA-7672: Restoring tasks need to be closed upon task suspension (apache#6113)
  KAFKA-7864; validate partitions are 0-based (apache#6246)
  KAFKA-7492 : Updated javadocs for aggregate and reduce methods returning null behavior. (apache#6285)
  ...
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@dongjinleekr @pengxiaolong
[TRIVIAL] Remove unused StreamsGraphNode#repartitionRequired (apache#…
3f88ca9 
…6227)

I found this defect while inspecting [KAFKA-7293: Merge followed by groupByKey/join might violate co-partitioning](https://issues.apache.org/jira/browse/KAFKA-7293); This flag is never used now. Instead, `KStreamImpl#repartitionRequired` is now covering its functionality.

Reviewers: Matthias J. Sax <mjsax@apache.org>, Bill Bejeck <bbejeck@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbejeck bbejeck bbejeck approved these changes

@mjsax mjsax mjsax approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dongjinleekr @mjsax @bbejeck