You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/04/07 01:39:36 UTC
[GitHub] [kafka] ddrid opened a new pull request, #12006: KAFKA-13794: Follow up to fix comparator
ddrid opened a new pull request, #12006:
URL: https://github.com/apache/kafka/pull/12006
Follow up to original PR #11991
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ddrid commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ddrid commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1090993473
also cc @hachikuji
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
artemlivshits commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r856590081
##########
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:
From the release perspective, it improves over the previous release, so for now we could take this as is and file a bug to work on a more precise solution in a future release. We could either add a unique long id (for every new object just increment a static atomic and get a new value) or instead of a producer batch use a HashSet of batches.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
artemlivshits commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850080492
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private static class TopicPartitionEntry {
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 Integer.compare(b1.hashCode(), b2.hashCode());
Review Comment:
Replied to the other comment as well. I wouldn't say the assumptions are incorrect -- the assumption is that hashCode would provide enough differentiation to significantly improve over original logic, without introducing regressions or requiring more complex implementation.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850003757
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private static class TopicPartitionEntry {
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 Integer.compare(b1.hashCode(), b2.hashCode());
Review Comment:
Hmm, `hashCode` can have collisions. This doesn't seem right either.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
showuon merged PR #12006:
URL: https://github.com/apache/kafka/pull/12006
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ddrid commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ddrid commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r844881404
##########
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:
Hi @showuon, thanks for you comment. I found some trouble when adding `equals` method in `ProducerBatch`. The only two final fields I can use is `createdMs` and `topicPartition`, I don't think it make sense if I use these two fields since they usually have duplicates. Do you have some suggestions? Thanks
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r860887484
##########
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:
It turns out that there are a couple of simple solutions, see #12096.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850032346
##########
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:
This is incorrect @artemlivshits, `Object.hashCode` is not based on object addresses in Java. The hashCode is 32 bits and object addresses can be 64-bits. Also, object addresses resulted in more collisions when used with hashmaps than the default implementation (which uses a random number generator).
I have seen collisions in practice that resulted in a huge amount of work to debug and diagnose. If you're interested, this was the fix https://github.com/squeryl/squeryl/commit/6e43673b273c270a39ba887ec9a07d921b566562
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850029605
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private static class TopicPartitionEntry {
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 Integer.compare(b1.hashCode(), b2.hashCode());
Review Comment:
I had the same question like you. But then, I think since we can't have a perfect way to compare two `ProducerBatch` object, the `hashCode` could be an alternative approach. That's my thought. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ddrid commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ddrid commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1090991057
Hi @artemlivshits, please take a look if you have time
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850032346
##########
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:
This is incorrect @artemlivshits, `Object.hashCode` is not based on object addresses in the most widely used Java implementation. The hashCode is 32 bits and object addresses can be 64-bits. Also, object addresses resulted in more collisions when used with hashmaps than the default implementation (which uses a random number generator).
I have seen collisions in practice that resulted in a huge amount of work to debug and diagnose. If you're interested, this was the fix https://github.com/squeryl/squeryl/commit/6e43673b273c270a39ba887ec9a07d921b566562
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
artemlivshits commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850094266
##########
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:
If equals is true, then hash codes must be equal too, so comparing hashCode for equal objects would return 0 in that case. If equals is false, then we do hashCode comparison. If we fold these 2 cases, then if we just do hashCode comparison, the result is the same as comparing equals first.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ddrid commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ddrid commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r844881404
##########
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:
Hi @showuon, thanks for you comment. I found some trouble when adding `equals` method in `ProducerBatch`. The only two final fields I can use is `createdMs` and `topicPartition`, I don't think it make sense if I use these two fields since they usually have duplicates. Do you have any suggestions? Thanks
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ddrid commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ddrid commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r845672887
##########
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:
@artemlivshits done, thanks for your suggestion
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850109104
##########
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:
That's true, we can't rely on hashcode if equals is false either. I'll take a look at the code a bit later, but it should be possible to avoid the latent bug here.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] sql888 commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by "sql888 (via GitHub)" <gi...@apache.org>.
sql888 commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1502205546
> Failed tests are unrelated
>
> ```
> Build / ARM / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
> ```
How to fix this failed test? getting this in M1 ARM Macbook. Thanks
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1093618609
Failed tests are unrelated
```
Build / ARM / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r844630559
##########
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:
Do you think we should override the `equals` and `hashcode` method in `ProducerBatch` class? Otherwise, the hashcode result could have possibility to be identical and makes the original issue happened again.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
artemlivshits commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1093103372
LGTM, thank you for the fix.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
artemlivshits commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r845433136
##########
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:
`ProducerBatch` doesn't override hashCode, so a default implementation is used. It's not fully specified what the default implementation of hashCode has to return, but it looks like the suggested implementation is to return a value based on the object address, so it should be different for different objects.
BTW, we could probably just do
```
else return Integer.compare(b1.hashCode(), b2.hashCode());
```
if the objects are equal, their hash code must be equal too.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850033183
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private static class TopicPartitionEntry {
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 Integer.compare(b1.hashCode(), b2.hashCode());
Review Comment:
I replied to the original discussion clarifying that some of the assumptions there are incorrect. We should not return `true` for the case where `hashCode` is true and `equals` is false.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850086332
##########
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:
But why do we have to limit ourselves to these 3 options? We can do the equals check and then do the hashCode thing if unequal.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1112200967
@tombentley See #12096.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1093624469
Cherry-pick back to 3.2, 3.1, 3.0 as https://github.com/apache/kafka/pull/11991 did.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
dajac commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1093839799
@tombentley We might want to get this one into 3.1.1 to fix the previous fix.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r845677599
##########
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:
Make sense. Thanks for the update.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850032346
##########
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:
This is incorrect @artemlivshits, `Object.hashCode` is not based on object addresses in Java. The hashCode is 32 bits and object addresses can be 64-bits. Also, object addresses resulted in more collisions when used with hashmaps than the default implementation.
I have seen collisions in practice that resulted in a huge amount of work to debug and diagnose. If you're interested, this was the fix https://github.com/squeryl/squeryl/commit/6e43673b273c270a39ba887ec9a07d921b566562
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850033183
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private static class TopicPartitionEntry {
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 Integer.compare(b1.hashCode(), b2.hashCode());
Review Comment:
I replied to the original discussion clarifying that some of the assumptions there are incorrect. We should not return `true` for the case where `hashCode` is true and `equal` is false.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a diff in pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #12006:
URL: https://github.com/apache/kafka/pull/12006#discussion_r850086685
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -187,7 +187,7 @@ private static class TopicPartitionEntry {
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 Integer.compare(b1.hashCode(), b2.hashCode());
Review Comment:
The assumption about object addresses and hashCode is what I was referring to.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
tombentley commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1104214543
@ijuma did you have any ideas about how to fix this without the problem with hash collisions?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12006: KAFKA-13794: Follow up to fix comparator
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12006:
URL: https://github.com/apache/kafka/pull/12006#issuecomment-1105486088
@tombentley I had a chat with @hachikuji about this and it seems that the behavior we want is slightly differently. I'll see if I can submit a PR today for discussion.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org