KAFKA-18019: Make INVALID_PRODUCER_ID_MAPPING a fatal error#17822
Merged
jolshan merged 4 commits intoNov 18, 2024
Merged
Conversation
jolshan
reviewed
Nov 14, 2024
CalvinLiu7947
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR! Left some minor comments
| enqueueRequest(handler); | ||
|
|
||
| // If an epoch bump is required for recovery, initialize the transaction after completing the EndTxn request. | ||
| if (epochBumpRequired) { |
Contributor
There was a problem hiding this comment.
Just to double check. The long-term goal is to only call initProducerId request when initializing the producer and we will not call it again when the producer is running. I guess it will be a separate change?
Contributor
There was a problem hiding this comment.
It'll be a separate change, because for for old protocol we still need to do the bump explicitly on the client. For the new protocol, we won't need to to bump from the client.
artemlivshits
approved these changes
Nov 15, 2024
| enqueueRequest(handler); | ||
|
|
||
| // If an epoch bump is required for recovery, initialize the transaction after completing the EndTxn request. | ||
| if (epochBumpRequired) { |
Contributor
There was a problem hiding this comment.
It'll be a separate change, because for for old protocol we still need to do the bump explicitly on the client. For the new protocol, we won't need to to bump from the client.
chiacyu
pushed a commit
to chiacyu/kafka
that referenced
this pull request
Nov 30, 2024
…7822) This patch contains changes to the handling of the INVALID_PRODUCER_ID_MAPPING error. Quoted from KIP-890 Since we bump epoch on abort, we no longer need to call InitProducerId to fence requests. InitProducerId will only be called when the producer starts up to fence a previous instance. With this change, some other calls to InitProducerId were inspected including the call after receiving an InvalidPidMappingException. This exception was changed to abortable as part of KIP-360: Improve reliability of idempotent/transactional producer. However, this change means that we can violate EOS guarantees. As an example: Consider an application that is copying data from one partition to another Application instance A processes to offset 4 Application instance B comes up and fences application instance A Application instance B processes to offset 5 Application instances A and B are idle for transaction.id.expiration.ms, transaction id expires on server Application instance A attempts to process offset 5 (since in its view, that is next) -- if we recover from invalid pid mapping, we can duplicate this processing Thus, INVALID_PID_MAPPING should be fatal to the producer. This is consistent with KIP-1050: Consistent error handling for Transactions where errors that are fatal to the producer are in the "application recoverable" category. This is a grouping that indicates to the client that the producer needs to restart and recovery on the application side is necessary. KIP-1050 is approved so we are consistent with that decision. This PR also fixes the flakiness of TransactionsExpirationTest. Reviewers: Artem Livshits <alivshits@confluent.io>, Justine Olshan <jolshan@confluent.io>, Calvin Liu <caliu@confluent.io>
tedyu
pushed a commit
to tedyu/kafka
that referenced
this pull request
Jan 6, 2025
…7822) This patch contains changes to the handling of the INVALID_PRODUCER_ID_MAPPING error. Quoted from KIP-890 Since we bump epoch on abort, we no longer need to call InitProducerId to fence requests. InitProducerId will only be called when the producer starts up to fence a previous instance. With this change, some other calls to InitProducerId were inspected including the call after receiving an InvalidPidMappingException. This exception was changed to abortable as part of KIP-360: Improve reliability of idempotent/transactional producer. However, this change means that we can violate EOS guarantees. As an example: Consider an application that is copying data from one partition to another Application instance A processes to offset 4 Application instance B comes up and fences application instance A Application instance B processes to offset 5 Application instances A and B are idle for transaction.id.expiration.ms, transaction id expires on server Application instance A attempts to process offset 5 (since in its view, that is next) -- if we recover from invalid pid mapping, we can duplicate this processing Thus, INVALID_PID_MAPPING should be fatal to the producer. This is consistent with KIP-1050: Consistent error handling for Transactions where errors that are fatal to the producer are in the "application recoverable" category. This is a grouping that indicates to the client that the producer needs to restart and recovery on the application side is necessary. KIP-1050 is approved so we are consistent with that decision. This PR also fixes the flakiness of TransactionsExpirationTest. Reviewers: Artem Livshits <alivshits@confluent.io>, Justine Olshan <jolshan@confluent.io>, Calvin Liu <caliu@confluent.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch contains changes to the handling of the INVALID_PRODUCER_ID_MAPPING error.
Quoted from KIP-890
Since we bump epoch on abort, we no longer need to call InitProducerId to fence requests. InitProducerId will only be called when the producer starts up to fence a previous instance.
With this change, some other calls to InitProducerId were inspected including the call after receiving an InvalidPidMappingException. This exception was changed to abortable as part of KIP-360: Improve reliability of idempotent/transactional producer. However, this change means that we can violate EOS guarantees. As an example:
Consider an application that is copying data from one partition to another
Application instance A processes to offset 4
Application instance B comes up and fences application instance A
Application instance B processes to offset 5
Application instances A and B are idle for transaction.id.expiration.ms, transaction id expires on server
Application instance A attempts to process offset 5 (since in its view, that is next) -- if we recover from invalid pid mapping, we can duplicate this processing
Thus, INVALID_PID_MAPPING should be fatal to the producer.
This is consistent with KIP-1050: Consistent error handling for Transactions where errors that are fatal to the producer are in the "application recoverable" category. This is a grouping that indicates to the client that the producer needs to restart and recovery on the application side is necessary. KIP-1050 is approved so we are consistent with that decision.