KAFKA-18672; CoordinatorRecordSerde must validate value version #18749
Conversation
This file contains 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
github-actions
bot
added
core
dajac
commented
Jan 30, 2025
Comment on lines
+80
to
+82
| if (valueVersion < valueMessage.lowestSupportedVersion() || valueVersion > valueMessage.highestSupportedVersion()) { | ||
| throw new UnknownRecordVersionException(recordType, valueVersion); | ||
| } |
There was a problem hiding this comment.
The UnknownRecordTypeException is caught as the aborted upgrade might leave behind some "new" records. Should we also consider handling UnknownRecordVersionException?
There was a problem hiding this comment.
That's a good question. I think that we would need to do a new KIP or to extend KIP-915: Txn and Group Coordinator Downgrade Foundation for this. In KIP-915, we defined that using tagged fields should be the way forward to handle backward compatibility. Hence, I am not sure that ignore newer version is needed.
dajac
commented
Jan 30, 2025
| * UnknownRecordTypeException is thrown when the Deserializer encounters | ||
| * an unknown record type. | ||
| */ | ||
| class UnknownRecordTypeException extends RuntimeException { |
There was a problem hiding this comment.
I just moved this one from the Loader to here.
3 tasks
jeffkbkim
reviewed
Jan 30, 2025
...inator/src/test/java/org/apache/kafka/coordinator/group/GroupCoordinatorRecordSerdeTest.java
Outdated
Show resolved
Hide resolved
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
CoordinatorRecordSerde does not validate the version of the value to check whether the version is supported by the current version of the software. This is problematic if a future and unsupported version of the record is read by an older version of the software because it would misinterpret the bytes. Hence CoordinatorRecordSerde must throw an error if the version is unknown. This is also consistent with the handling in the old coordinator.
Committer Checklist (excluded from commit message)