artemlivshits commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850078629
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private void startSequencesAtBeginning(TopicPartition
topicPartition, ProducerId
private static final Comparator<ProducerBatch>
PRODUCER_BATCH_COMPARATOR = (b1, b2) -> {
if (b1.baseSequence() < b2.baseSequence()) return -1;
else if (b1.baseSequence() > b2.baseSequence()) return 1;
- else return b1.equals(b2) ? 0 : 1;
+ else return b1.equals(b2) ? 0 : Integer.compare(b1.hashCode(),
b2.hashCode());
Review Comment:
In this case, even if hashCode always returned 0, it's as good as the
original implementation, where any batch with the same baseSequence would be
treated as equal and the consequences are clear and deterministic. Returning 1
when it's not equal may lead to issues that are super hard to debug --
sometimes an element could be found, and sometimes could not (I'd say only race
conditions would beat binary search in unsorted container :-)).
In general -- agree that hashCode is not fully specified, so if we really
want to make sure that different batches have truly different identities and
keep them ordered, we'd need to come up with a stable way to order them, e.g.
stamp each batch with a unique integer. (In C++ I would've just used the
address, I couldn't find a built-in way to order 2 objects in Java).
From the release management perspective, here is my regression ranking:
1. Comparing just baseSequence was there for 3 years, so probably not super
severe.
2. This fix partially (fairly reliably in practice) fixes the original
problem, no regression.
3. Returning 1 when not equal (#11991) introduces a hard to debug problem.
If I had to pick between the three, 2 is a clear winner -- makes the
original behavior better, without introducing regressions.
--
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]
