Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Add unit test for KAFKA-8676 to guard against unrequired task restarts #7287

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@kkonstantine
Copy link
Contributor

commented Sep 3, 2019

After KIP-415 requesting restart only of the affected connector tasks is required to avoid unnecessary task restarts. This PR adds a unit test to guard against changes in this behavior.

The original fix is introduced by the PR prefixed with KAFKA-8676

Committer Checklist (excluded from commit message)

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@rhauch @guozhangwang
This tests the fix introduced here:
#7097

Should succeed once the above fix is merged.

@rhauch
rhauch approved these changes Sep 3, 2019
Copy link
Contributor

left a comment

LGTM. Thanks, @kkonstantine, for providing a test case for #7097. I verified that this new unit tests fails without the change in #7097, but succeeds once that change is made. And the new unit test does appear to verify the logic correctly.

I plan to merge #7097 first and then apply this change. I've verified locally that applying #7097 and this PR together makes this new unit test pass and does not break other unit tests.

"tasks", 1); // Starts with 2 tasks, after update has 3

// As soon as root is rewritten, we should see a callback notifying us that we reconfigured some tasks
configUpdateListener.onTaskConfigUpdate(Arrays.asList(TASK_IDS.get(2)));

This comment has been minimized.

Copy link
@rhauch

rhauch Sep 3, 2019

Contributor

This is the essential line of the test that verifies the change in #7097. Without the fix in #7097, the listener will get an update with all 3 task IDs, and to pass this line would then need to be changed to:

        configUpdateListener.onTaskConfigUpdate(Arrays.asList(TASK_IDS.get(2), TASK_IDS.get(0), TASK_IDS.get(1)));

However, we don't want all task IDs to be updated. Instead only want only the task ID(s) for the tasks that were indeed updated. That's why this test case expects that the task config update only includes the one ID of the task that is actually updated (i.e., TASK_IDS.get(2)). So as is, this test method will fail unless the fix for #7097 is actually applied.

This comment has been minimized.

Copy link
@kkonstantine

kkonstantine Sep 3, 2019

Author Contributor

That's right. That was my intention. Thanks for verifying

@rhauch rhauch merged commit 74fc332 into apache:trunk Sep 3, 2019

0 of 3 checks passed

JDK 11 and Scala 2.12 Build triggered for merge commit.
Details
JDK 11 and Scala 2.13 Build triggered for merge commit.
Details
JDK 8 and Scala 2.11 Build triggered for merge commit.
Details

@kkonstantine kkonstantine deleted the kkonstantine:minor-unit-test-for-kafka-8676 branch Sep 3, 2019

rhauch added a commit that referenced this pull request Sep 3, 2019
MINOR: Add unit test for KAFKA-8676 to guard against unrequired task …
…restarts (#7287)

Added unit test for recent fix of `KafkaConfigBackingStore`.

Author: Konstantine Karantasis <konstantine@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.