Skip to content

KAFKA-7833: Add StateStore name conflict check in InternalTopologyBuilder#8825

Merged
mjsax merged 1 commit into
apache:trunkfrom
chebbyChefNEQ:rmeng/node-name-conflict-check
Jun 11, 2020
Merged

KAFKA-7833: Add StateStore name conflict check in InternalTopologyBuilder#8825
mjsax merged 1 commit into
apache:trunkfrom
chebbyChefNEQ:rmeng/node-name-conflict-check

Conversation

@chebbyChefNEQ

@chebbyChefNEQ chebbyChefNEQ commented Jun 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

*Summary of testing strategy (including rationale)

  • Added two tests that calls addGlobalStore and addStateStore in two different orders
  • Added check for name conflict with a global store in the addStateStore path

Committer Checklist (excluded from commit message)

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

@chebbyChefNEQ chebbyChefNEQ changed the title KAFKA-7833: Add name conflict check in StreamsGraphNode KAFKA-7833: Add StateStore name conflict check in InternalStreamsBuilder Jun 7, 2020
@mjsax

mjsax commented Jun 7, 2020

Copy link
Copy Markdown
Member

Thanks for the PR. I looked into the code and the check to verify if names are overlapping is actually in InternalTopologyBuilder -- thus, the fix should there.

In InternalTopologyBuilderTest, there is already shouldNotAllowToAddStoresWithSameName - - but it only covers the case for two "regular" store. The bug is really for adding a regular store and global store with the same name. Seems, we also lack a test for adding two global stores with the same names.

Also note, that for adding a regular store first, and afterwards a global store, there seems to be not bug atm -- only if the global store is added first and the regular store second the issue is there. Thus, we should test both combinations.

Does this make sense?

@mjsax mjsax added the streams label Jun 7, 2020
@chebbyChefNEQ

chebbyChefNEQ commented Jun 7, 2020

Copy link
Copy Markdown
Contributor Author

@mjsax Makes sense. I'll push out the change tonight 👍

@chebbyChefNEQ
chebbyChefNEQ force-pushed the rmeng/node-name-conflict-check branch from caf4fb7 to 3ed345b Compare June 8, 2020 05:40
@chebbyChefNEQ chebbyChefNEQ changed the title KAFKA-7833: Add StateStore name conflict check in InternalStreamsBuilder KAFKA-7833: Add StateStore name conflict check in InternalTopologyBuilder Jun 8, 2020
@mjsax

mjsax commented Jun 10, 2020

Copy link
Copy Markdown
Member

Retest this please.

1 similar comment
@mjsax

mjsax commented Jun 10, 2020

Copy link
Copy Markdown
Member

Retest this please.

@chebbyChefNEQ

chebbyChefNEQ commented Jun 10, 2020

Copy link
Copy Markdown
Contributor Author

@mjsax I might have misread something somewhere. How do I trigger test explicitly? https://cwiki.apache.org/confluence/display/KAFKA/Contributing+Code+Changes seems to suggest that tests are automatically triggered on PRs.

@mjsax

mjsax commented Jun 10, 2020

Copy link
Copy Markdown
Member

Thanks for pointing it out. The info is not correct any more. I just updated the wiki.

ASF infra team decided to protect Jenkins and now builds need to be triggered by committers.

@mjsax
mjsax merged commit 3e9ba22 into apache:trunk Jun 11, 2020
mjsax pushed a commit that referenced this pull request Jun 11, 2020
@chebbyChefNEQ @mjsax
KAFKA-7833: Add Global/StateStore name conflict check (#8825)
106a3d7 
Reviewers: Matthias J. Sax <matthias@confluent.io>
@mjsax

mjsax commented Jun 11, 2020

Copy link
Copy Markdown
Member

Thanks for the PR! Merged to trunk and cherry-picked to 2.6 branch.

@mjsax mjsax mentioned this pull request Jun 11, 2020
Merged
Kvicii pushed a commit to Kvicii/kafka that referenced this pull request Jun 13, 2020
Merge branch 'trunk' of github.com:apache/kafka into trunk-read
49b7b7f 
* 'trunk' of github.com:apache/kafka: (42 commits)
  HOTFIX: Fix compile error in TopicAdminTest (apache#8866)
  KAFKA-10144: clean up corrupted standby tasks before attempting a commit (apache#8849)
  KAFKA-10157: Fix broken tests due to InterruptedException from FinalizedFeatureChangeListener (apache#8857)
  KAFKA-9432: automated protocol for DescribeConfigs (apache#8312)
  KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764)
  KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680)
  KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787)
  KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818)
  KAFKA-9374: Make connector interactions asynchronous (apache#8069)
  MINOR: reduce sizeInBytes for percentiles metrics (apache#8835)
  KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829)
  KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828)
  KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455)
  KAFKA-7833: Add missing test (apache#8847)
  KAFKA-9066: Retain metrics for failed tasks (apache#8502)
  KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453)
  KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663)
  KAFKA-9441: remove prepareClose() to simplify task management (apache#8833)
  KAFKA-7833: Add Global/StateStore name conflict check (apache#8825)
  KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630)
  ...
ijuma added a commit to ijuma/kafka that referenced this pull request Nov 17, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
1869826 
…t-for-generated-requests

* apache-github/trunk: (248 commits)
  KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764)
  KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680)
  KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787)
  KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818)
  KAFKA-9374: Make connector interactions asynchronous (apache#8069)
  MINOR: reduce sizeInBytes for percentiles metrics (apache#8835)
  KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829)
  KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828)
  KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455)
  KAFKA-7833: Add missing test (apache#8847)
  KAFKA-9066: Retain metrics for failed tasks (apache#8502)
  KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453)
  KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663)
  KAFKA-9441: remove prepareClose() to simplify task management (apache#8833)
  KAFKA-7833: Add Global/StateStore name conflict check (apache#8825)
  KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630)
  KAFKA-9991: Fix flaky unit tests (apache#8843)
  KAFKA-10014; Always try to close all channels in Selector#close (apache#8685)
  KAFKA-10079: improve thread-level stickiness (apache#8775)
  MINOR: Print all removed dynamic members during join complete (apache#8816)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chebbyChefNEQ @mjsax