Skip to content

KAFKA-9850 Move KStream#repartition operator validation during Topolo…#8550

Merged
mjsax merged 5 commits into
apache:trunkfrom
zhaohaidao:trunk
May 14, 2020
Merged

KAFKA-9850 Move KStream#repartition operator validation during Topolo…#8550
mjsax merged 5 commits into
apache:trunkfrom
zhaohaidao:trunk

Conversation

@zhaohaidao

@zhaohaidao zhaohaidao commented Apr 25, 2020

Copy link
Copy Markdown
Contributor

Tickets: KAFKA-9850

  • Add repartition operator validation during topology.build()
  • Move one repartition operator validation test from IntegrationTest to UnitTest

@abbccdda abbccdda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the PR, get some comments.

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

@abbccdda Hi, pr updated. Could you continue to review it?

@abbccdda

Copy link
Copy Markdown

@mjsax @vvcephei I think this change makes sense, could you two also take a look?

@mjsax mjsax added the streams label May 1, 2020
.collect(Collectors.toSet())
).collect(Collectors.toList());
for (final Set<String> copartition : copartitionGroups) {
final Map<String, Integer> topicPartNum = new HashMap<>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is topicPartNum? Please avoid abbreviation; make the code hard to read?

Should this be numberOfPartitionsPerTopic ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done and thanks for your advice.

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Made an initial pass.

@mjsax

mjsax commented May 2, 2020

Copy link
Copy Markdown
Member

Retest this please.

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

Retest this please.

@mjsax I have modified the codes according to your suggestion and passed the test. Could you continue to review it?

@mjsax

mjsax commented May 2, 2020

Copy link
Copy Markdown
Member

Thanks for the update. I'll wait for Jenkins and do another pass afterwards.

Btw: the comment "Retest this please" is not for you, but for Jenkins to run the build :) -- you can ignore those comments. (Also note, only committers can trigger builds so it won't work for you).

@mjsax

mjsax commented May 2, 2020

Copy link
Copy Markdown
Member

Retest this please.

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

@mjsax hi, failed tests have been fixed. Could you retest this?

@mjsax

mjsax commented May 8, 2020

Copy link
Copy Markdown
Member

Retest this please.

@mjsax

mjsax commented May 8, 2020

Copy link
Copy Markdown
Member

Will try to review in the next couple of day.

@zhaohaidao

Copy link
Copy Markdown
Contributor Author

Retest this please.

@mjsax could you retest this? I tested cases in my laptop and the failed case passed

@mjsax
mjsax merged commit 43a9e39 into apache:trunk May 14, 2020
@mjsax

mjsax commented May 14, 2020

Copy link
Copy Markdown
Member

Thanks for the PR @zhaohaidao! Merged to trunk.

Kvicii added a commit to Kvicii/kafka that referenced this pull request May 14, 2020
* 'trunk' of github.com:apache/kafka:
  MINOR: add option to rebuild source for system tests (apache#6656)
  KAFKA-9850 Move KStream#repartition operator validation during Topolo… (apache#8550)
  MINOR: Add a duplicate() method to Message classes (apache#8556)
  KAFKA-9966: add internal assignment listener to stabilize eos-beta upgrade test (apache#8648)
  MINOR: Replace null with an actual value for timestamp field in InsertField SMT unit tests (apache#8649)
  MINOR: Fix ProcessorContext JavaDocs and stream-time computation (apache#8603)
  MINOR: improve tests for TopologyTestDriver (apache#8631)
  KAFKA-9821: consolidate Streams rebalance triggering mechanisms (apache#8596)
  KAFKA-9669; Loosen validation of inner offsets for older message formats (apache#8647)
  KAFKA-8770: KIP-557: Drop idempotent KTable source updates (apache#8254)
  MINOR: Remove allow concurrent test (apache#8641)
jwijgerd pushed a commit to buxapp/kafka that referenced this pull request May 14, 2020
apache#8550)

Reviewers: Boyang Chen <boyang@confluent.io>, Matthias J. Sax <matthias@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants