KAFKA-8002: Replica reassignment to new log dir may not complete if f… by bob-barrett · Pull Request #6346 · apache/kafka

bob-barrett · 2019-02-28T18:01:59Z

…uture and current replicas segment files have different base offsets

This patch fixes a bug in log dir reassignment where Partition.maybeReplaceCurrentWithFutureReplica would compare the entire LogEndOffsetMetadata of each replica to determine whether the reassignment has completed. If the active segments of both replicas have different base segments (for example, if the current replica had previously been cleaned and the future replica rolled segments at different points), the reassignment will never complete. The fix is to compare only the LogEndOffsetMetadata.messageOffset for each replica. Tested with a unit test that simulates the compacted current replica case.

Committer Checklist (excluded from commit message)

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

…uture and current replicas segment files have different base offsets This patch fixes a bug in log dir reassignment where Partition.maybeReplaceCurrentWithFutureReplica would compare the entire LogEndOffsetMetadata of each replica to determine whether the reassignment has completed. If the active segments of both replicas have different base segments (for example, if the current replica had previously been cleaned and the future replica rolled segments at different points), the reassignment will never complete. The fix is to compare only the LogEndOffsetMetadata.messageOffset for each replica. Tested with a unit test that simulates the compacted current replica case.

bob-barrett · 2019-02-28T18:44:45Z

retest this please

hachikuji

Thanks, looks good overall. Left a minor suggestion.

hachikuji · 2019-02-28T19:43:15Z

        futureLocalReplica match {
          case Some(futureReplica) =>
-            if (replica.logEndOffset == futureReplica.logEndOffset) {
+            if (replica.logEndOffset.messageOffset == futureReplica.logEndOffset.messageOffset) {


One thing I was discussing with @apovzner is whether we should consider renaming logEndOffset. The bug probably would never have occurred if we had a better name. Maybe we should have two methods:

def logEndOffset: Long def logEndOffsetMetadata: LogOffsetMetadata

I like the suggestion of having logEndOffsetMetadata method. Although, need to be careful not to miss any calls that would need to be renamed to
logEndOffsetMetadata (and get to the same type of bug we found here). I guess one way to make sure this does not happen is to first change the name of logEndOffset to
logEndOffsetMetadata, make sure to compile, and only after that add
logEndOffset that returns Long.

hachikuji · 2019-02-28T19:44:31Z

  }

+  @Test
+  // Verify that replacement works when the replicas have the same log end offset but different base offsets in the


nit: Move this comment above annotation?

apovzner · 2019-02-28T20:53:15Z

Thanks for the PR! Looks good -- I don't have anything to add to Jason's comments.

…fset return the message offset

hachikuji

LGTM. Thanks for the fix!

…nt segment base offset (#6346) This patch fixes a bug in log dir reassignment where Partition.maybeReplaceCurrentWithFutureReplica would compare the entire LogEndOffsetMetadata of each replica to determine whether the reassignment has completed. If the active segments of both replicas have different base segments (for example, if the current replica had previously been cleaned and the future replica rolled segments at different points), the reassignment will never complete. The fix is to compare only the LogEndOffsetMetadata.messageOffset for each replica. Tested with a unit test that simulates the compacted current replica case. Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>

… has different segment base offset (apache#6346) This patch fixes a bug in log dir reassignment where Partition.maybeReplaceCurrentWithFutureReplica would compare the entire LogEndOffsetMetadata of each replica to determine whether the reassignment has completed. If the active segments of both replicas have different base segments (for example, if the current replica had previously been cleaned and the future replica rolled segments at different points), the reassignment will never complete. The fix is to compare only the LogEndOffsetMetadata.messageOffset for each replica. Tested with a unit test that simulates the compacted current replica case. backported to 2.0-li Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>

* apache/trunk: KAFKA-7880:Naming worker thread by task id (apache#6275) improve some logging statements (apache#6078) KAFKA-7312: Change broker port used in testMinimumRequestTimeouts and testForceClose KAFKA-7997: Use automatic RPC generation in SaslAuthenticate KAFKA-8002; Log dir reassignment stalls if future replica has different segment base offset (apache#6346) KAFKA-3522: Add TimestampedKeyValueStore builder/runtime classes (apache#6152) HOTFIX: add igore import to streams_upgrade_test MINOR: ConsumerNetworkClient does not need to send the remaining requests when the node is not ready (apache#6264) KAFKA-7922: Return authorized operations in describe consumer group responses (KIP-430 Part-1) KAFKA-7918: Inline generic parameters Pt. III: in-memory window store (apache#6328) KAFKA-8012; Ensure partitionStates have not been removed before truncating. (apache#6333) KAFKA-8011: Fix for race condition causing concurrent modification exception (apache#6338) KAFKA-7912: Support concurrent access in InMemoryKeyValueStore (apache#6336) MINOR: Skip quota check when replica is in sync (apache#6344) HOTFIX: Change header back to http instead of https to path license header test (apache#6347) MINOR: fix release.py script (apache#6317) MINOR: Remove types from caching stores (apache#6331) MINOR: Improve logging for alter log dirs (apache#6302) MINOR: state.cleanup.delay.ms default is 600,000 ms (10 minutes). (apache#6345) MINOR: disable Streams system test for broker upgrade/downgrade (apache#6341)

…nt segment base offset (apache#6346) This patch fixes a bug in log dir reassignment where Partition.maybeReplaceCurrentWithFutureReplica would compare the entire LogEndOffsetMetadata of each replica to determine whether the reassignment has completed. If the active segments of both replicas have different base segments (for example, if the current replica had previously been cleaned and the future replica rolled segments at different points), the reassignment will never complete. The fix is to compare only the LogEndOffsetMetadata.messageOffset for each replica. Tested with a unit test that simulates the compacted current replica case. Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>

bob-barrett added 2 commits February 28, 2019 09:57

Fix Scala 2.11 compile error

3fcc866

hachikuji reviewed Feb 28, 2019

View reviewed changes

Rename Replica.logEndOffset to logEndOffsetMetadata and make logEndOf…

8d5dee0

…fset return the message offset

hachikuji approved these changes Mar 2, 2019

View reviewed changes

hachikuji merged commit 57604c2 into apache:trunk Mar 2, 2019

bob-barrett deleted the KAFKA-8002 branch March 4, 2019 07:46

bob-barrett restored the KAFKA-8002 branch March 4, 2019 07:47

bob-barrett deleted the KAFKA-8002 branch March 4, 2019 07:56

bob-barrett restored the KAFKA-8002 branch March 4, 2019 07:57

bob-barrett deleted the KAFKA-8002 branch March 4, 2019 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-8002: Replica reassignment to new log dir may not complete if f…#6346

KAFKA-8002: Replica reassignment to new log dir may not complete if f…#6346
hachikuji merged 3 commits intoapache:trunkfrom
bob-barrett:KAFKA-8002

bob-barrett commented Feb 28, 2019 •

edited

Loading

Uh oh!

bob-barrett commented Feb 28, 2019

Uh oh!

hachikuji left a comment

Uh oh!

hachikuji Feb 28, 2019

Uh oh!

apovzner Feb 28, 2019

Uh oh!

hachikuji Feb 28, 2019

Uh oh!

apovzner commented Feb 28, 2019

Uh oh!

hachikuji left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

bob-barrett commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

bob-barrett commented Feb 28, 2019

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

hachikuji Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

apovzner Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

hachikuji Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

apovzner commented Feb 28, 2019

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bob-barrett commented Feb 28, 2019 •

edited

Loading