-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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-15449: Verify transactional offset commits (KIP-890 part 1) #14370
Conversation
clients/src/main/java/org/apache/kafka/common/requests/ProduceResponse.java
Outdated
Show resolved
Hide resolved
clients/src/main/java/org/apache/kafka/common/requests/TxnOffsetCommitResponse.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jolshan Thanks for the PR. I left a few comments for consideration.
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/kafka/coordinator/group/GroupCoordinator.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala
Outdated
Show resolved
Hide resolved
case Errors.INVALID_PRODUCER_ID_MAPPING | ||
| Errors.INVALID_TXN_STATE => | ||
Errors.UNKNOWN_MEMBER_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the rational behind using UNKNOWN_MEMBER_ID here? We should also add a comment about it because this is not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I resolved the PR comment, but I had a discussion with myself here: #14370 (comment)
Similar to the produce request, we wanted to have a non-fatal error here. (I suppose there is an argument for the invalid PID mapping being fatal)
What do you think? There are only 2 abortable errors returned by this request.
} else if (error == Errors.UNKNOWN_MEMBER_ID
|| error == Errors.ILLEGAL_GENERATION) {
abortableError(new CommitFailedException("Transaction offset Commit failed " +
"due to consumer group metadata mismatch: " + error.exception().getMessage()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For posterity, we decided to not go with the abortable error and stick with the fatal one since it makes it clearer what happened. We also can't guarantee clients treat unknown member as non-fatal.
core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorConcurrencyTest.scala
Outdated
Show resolved
Hide resolved
core/src/test/scala/unit/kafka/coordinator/group/GroupCoordinatorTest.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a small nit. I don't need to re-review it.
core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala
Show resolved
Hide resolved
…ache#14370) Previous commits left out TxnOffsetCommits which go through the group coordinator (not directly from the producer). I've wired up the methods to include the transactional id and state partition to do the verification. I've also updated UnifiedLog to verify on client and coordinator requests that are transactional. I've not updated any sequence check logic since the sequence is always 0 on group coordinator initiated writes. Added returned errors to Response files. Both InvalidPidMapping and InvalidTxnState will be returned and be fatal for the transactional OffsetCommit requests. Reviewers: David Jacot <david.jacot@gmail.com>, Artem Livshits <alivshits@confluent.io>
…ache#14370) Previous commits left out TxnOffsetCommits which go through the group coordinator (not directly from the producer). I've wired up the methods to include the transactional id and state partition to do the verification. I've also updated UnifiedLog to verify on client and coordinator requests that are transactional. I've not updated any sequence check logic since the sequence is always 0 on group coordinator initiated writes. Added returned errors to Response files. Both InvalidPidMapping and InvalidTxnState will be returned and be fatal for the transactional OffsetCommit requests. Reviewers: David Jacot <david.jacot@gmail.com>, Artem Livshits <alivshits@confluent.io>
Previous commits left out TxnOffsetCommits which go through the group coordinator (not directly from the producer).
I've wired up the methods to include the transactional id and state partition to do the verification.
I've also updated UnifiedLog to verify on client and coordinator requests that are transactional.
I've not updated any sequence check logic since the sequence is always 0 on group coordinator initiated writes.
Committer Checklist (excluded from commit message)