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-9171: Handle ReplicaNotAvailableException during DelayedFetch #7678

Merged
merged 2 commits into from Nov 11, 2019

Conversation

rajinisivaram
Copy link
Contributor

Committer Checklist (excluded from commit message)

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

@@ -139,6 +140,9 @@ class DelayedFetch(delayMs: Long,
case _: NotLeaderForPartitionException => // Case A
debug("Broker is no longer the leader of %s, satisfy %s immediately".format(topicPartition, fetchMetadata))
return forceComplete()
case _: ReplicaNotAvailableException => // Case H
debug("Broker no longer has a replica of %s, satisfy %s immediately".format(topicPartition, fetchMetadata))
Copy link
Contributor

@ijuma ijuma Nov 11, 2019

Choose a reason for hiding this comment

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

Can we use string interpolation? We do it for 3 of the other cases, but not the one immediately above. Also, should this be case B and shift the rest instead of case H? It seems like it would be easier to understand if the first 3 cases were about leader/replica/partition knowledge. Also, we could have the case checks in order: A, B, C, etc.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ijuma Thanks for the review, updated.

Copy link
Contributor

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM. Just to double check, none of the exceptions in the catch block is a subclass of another exception in the same catch block, right? This ensures ordering is irrelevant.

@rajinisivaram
Copy link
Contributor Author

@ijuma Thanks for the review. Have confirmed that none of the exceptions in the catch block are subclasses of another exception in the catch block.

@rajinisivaram
Copy link
Contributor Author

@ijuma Thanks for the review, merging to trunk and 2.4.

@rajinisivaram rajinisivaram merged commit 6f00086 into apache:trunk Nov 11, 2019
rajinisivaram added a commit that referenced this pull request Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants