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

KAFKA-8676: Avoid Unnecessary stops and starts of tasks #7097

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@LuyingLiu
Copy link
Contributor

commented Jul 17, 2019

As this is testing for initialization behaviors, I do not think it is a
good idea to do unit and integrate testing. System tests are needed.
Log INFO output and KafkaConnect API for checking the status of
connectors and tasks are useful tools when checking which
connectors and tasks start/stop.

Committer Checklist (excluded from commit message)

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

@guozhangwang

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Just by reading the description I think the change looks reasonable, ping @rhauch @kkonstantine for another look.

Also, could you add a unit test covering this case to illustrate the behavioral difference?

@LuyingLiu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

I am having a hard time adding a unit test for this, as it is difficult to make sure that a particular task is not started. And I think it is a good idea to add an integration test to illustrate the difference of task-starts for a particular request.

@kkonstantine
Copy link
Contributor

left a comment

Thanks for the fix @LuyingLiu !

I've confirmed that it works. Currently it's evident only under the new compatible connect protocol, because in eager all the tasks get restarted anyways. I've written a unit test to guard against a change in this behavior which I pushed here: #7287 so I don't interfere with your current PR.

An integration test could be extended later on to validate this fix as well.

LGTM

@kkonstantine

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

@rhauch would you mind taking a look?
This is a good fix now that incremental cooperative rebalancing is the default in Connect.
It avoids unnecessary restarts during connector reconfiguration.

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

left a comment

LGTM. Thanks, @LuyingLiu, for reporting this and for providing the fix. Also thanks to @kkonstantine for supplying in #7287 a new unit test that verifies that the current behavior is incorrect and that the behavior introduced in this PR is correct.

I plan to merge this back to the 2.3 branch, which is when the affected code started relying upon notifications that only the changed tasks were updated. Prior to that, signaling that all tasks were changed was not a big deal because all tasks were stopped and restarted during a rebalance anyway, so if extra task IDs were considered updated then no big deal -- backporting this fix farther back would seem to add unnecessary risk.

@rhauch rhauch merged commit 31a5f92 into apache:trunk Sep 3, 2019

1 of 3 checks passed

JDK 11 and Scala 2.12 FAILURE 11669 tests run, 67 skipped, 2 failed.
Details
JDK 8 and Scala 2.11 FAILURE 11464 tests run, 67 skipped, 3 failed.
Details
JDK 11 and Scala 2.13 SUCCESS 11669 tests run, 67 skipped, 0 failed.
Details
rhauch added a commit that referenced this pull request Sep 3, 2019
Changed for updatedTasks, avoids stopping and starting of unnecessary…
… tasks (#7097)

Corrected the `KafkaConfigBackingStore` logic to notify of only the changed tasks, rather than all tasks. This was not noticed before because Connect always stopped and restarted all tasks during a rebalanced, but since 2.3 the incremental rebalance logic exposed this bug.

Author: Luying Liu <lyliu@lyliu-mac.freewheelmedia.net>
Reviewers: Konstantine Karantasis <konstantine@confluent.io>, 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
4 participants
You can’t perform that action at this time.