git-ai-coder commented on PR #23593: URL: https://github.com/apache/kafka/pull/23593#issuecomment-5875163707
Thank you for the thorough review, @squah-confluent! **On varints with the 32nd bit set:** We intentionally do not reject these in `readUnsignedVarint`. The existing `testUnsignedVarintSerde` already round-trips `Integer.MIN_VALUE` and `-1`. Rejecting negative values at this layer would be a breaking change. Length validation is the callers' responsibility. varints with the 32nd bit set (e.g. Integer.MIN_VALUE) intentionally decode to negative integers and are valid; readUnsignedVarint is not required to return only non-negative values, per existing usage in the codebase. Updated the PR description to clarify this. **On varlong unused-bit overflow:** varlong unused-bit overflow is not relevant because readUnsignedVarlong uses a loop-based decoder that already rejects any byte that has the continuation bit set past 63 bits; no separate final-byte check is needed. **On test patterns — you're right:** `testReadUnsignedVarintRejectsOverflow` was redundant given the existing `testInvalidVarint`. Merged the unused-bit overflow cases (both ByteBuffer and InputStream paths, including boundary cases for 0x1F, 0x10, and the valid 0x0F) directly into `testInvalidVarint` and removed the standalone method. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
