Skip to content

KAFKA-8449: Restart tasks on reconfiguration under incremental cooperative rebalancing#6850

Merged
rhauch merged 3 commits into
apache:trunkfrom
kkonstantine:kafka-8449
Jun 3, 2019
Merged

KAFKA-8449: Restart tasks on reconfiguration under incremental cooperative rebalancing#6850
rhauch merged 3 commits into
apache:trunkfrom
kkonstantine:kafka-8449

Conversation

@kkonstantine

Copy link
Copy Markdown
Contributor

With incremental cooperative rebalancing tasks are not restarted unless they are reassigned to different worker. Therefore, to load config updates due to connector reconfiguration the currently running tasks that get new configs need to be recorded and considered for restart when connectors and tasks are started after a rebalance.

Committer Checklist (excluded from commit message)

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

@kkonstantine

Copy link
Copy Markdown
Contributor Author

@rhauch @ewencp this PR aims to fix the issue with tasks that don't pick up new configs under reconfiguration. Your thoughts are appreciated

@ijuma
ijuma requested a review from rhauch May 31, 2019 15:56
@kkonstantine

Copy link
Copy Markdown
Contributor Author

jk8 tests green. jdk11 has one failure in a not relevant test case (kafka.network.DynamicConnectionQuotaTest.testDynamicConnectionQuota).

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @kkonstantine! These changes look good, but I have a few pretty minor suggestions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: it would help to have a comment here to explain why the tasks to restart are removed from the running tasks.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: this log line is in a different location in the similar updateConfigsWithEager(...) method. Would it make sense to move this log message to just before the member.requestRejoin() call?

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.

Moved it to both before member.requestRejoin. I changed it because it was even after we set this variable to false, which made the log message a bit confusing.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it make sense to move this log message to just before the member.requestRejoin() call?

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

…eager and cooperative

Also include the group generation in the log message when the worker receives its assignment
@kkonstantine

Copy link
Copy Markdown
Contributor Author

Thanks @rhauch !
I addressed your comments and pushed an update that keeps the two execution paths separate between eager and cooperative. Waiting for the unit tests and will run a round of system tests too.

@rhauch rhauch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks, @kkonstantine!

@rhauch
rhauch merged commit 3c7c988 into apache:trunk Jun 3, 2019
rhauch pushed a commit that referenced this pull request Jun 3, 2019
…ative rebalancing (#6850)

Restart task on reconfiguration under incremental cooperative rebalancing, and keep execution paths separate for config updates between eager and cooperative. Include the group generation in the log message when the worker receives its assignment.

Author: Konstantine Karantasis <konstantine@confluent.io>
Reviewer: Randall Hauch <rhauch@gmail.com>
omkreddy added a commit to confluentinc/kafka that referenced this pull request Jun 4, 2019
* AK_REPO/2.3:
  KAFKA-8155: Add 2.2.0 release to system tests (apache#6597)
  KAFKA-8404: Add HttpHeader to RestClient HTTP Request and Connector REST API (apache#6791)
  KAFKA-8473: Adjust Connect system tests for incremental cooperative rebalancing (apache#6872)
  KAFKA-8475: Temporarily restore SslFactory.sslContext() helper
  KAFKA-8449: Restart tasks on reconfiguration under incremental cooperative rebalancing (apache#6850)
  KAFKA-8426; Fix for keeping the ConfigProvider configs consistent with KIP-297 (apache#6750)
  KAFKA-8425: Fix for correctly handling immutable maps (KIP-421 bug) (apache#6795)
  MINOR: Reordering the props modification with configs construction
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…ative rebalancing (apache#6850)

Restart task on reconfiguration under incremental cooperative rebalancing, and keep execution paths separate for config updates between eager and cooperative. Include the group generation in the log message when the worker receives its assignment.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants