[Issue 9438][pulsar-client] Fix inconsistent equals and hashCode for MessageIds #9440

gmethvin · 2021-02-03T04:38:15Z

Motivation

Since MessageId is an opaque interface that has several subclasses, in general those subclasses should try to behave the same way with respect to one another if they refer to the same message. They should also follow the standard contract for Java equals and hashCode methods.

Modifications

Provided a single equals and hashCode implementation for MessageIdImpl that takes into account the batch version as well. Changed TopicMessageIdImpl to not take into account the topic anymore.

Verifying this change

Extended unit test for BatchMessageIdImpl
Added unit test for TopicMessageIdImpl

pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java

gmethvin · 2021-02-04T00:46:29Z

I also noticed the compareTo implementation seemed a bit inconsistent, so I tried to simplify it, but I'm not sure what the intention was exactly so I may not have implemented it properly.

sijie · 2021-02-08T05:03:47Z

@merlimat Can you review it again?

pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java

pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java

gmethvin · 2021-03-18T23:30:11Z

@codelipenghui @315157973 responded to your comments.

I get that the implementation for this is a little weird, so let me know if you have any other suggestions to achieve the same goal. Ideally we would have single message ID type that can represent either batched or unbatched messages, but I'm not sure how feasible that is without breaking changes.

eolivelli · 2021-03-19T08:01:43Z

Thinking out loud:

What about keeping your new tests cases, they express exactly the problem.
and fix the "equals" and "hashCode" methods of all of the implementations of MessageId in a way that all of the combinations work?
(it would be great to be able to mark MessageId as a 'sealed' interface).

in order to implement hashCode you could add a static utility method that receives all of the relevant fields:

public static int computeMessageIdHashCode(long ledgerId, long entryId, long partitionIndex, long batchIndex)

the same can be done for the equals method

public static booleanMessageIdEquals(long ledgerId1, long entryId1, long partitionIndex1, long batchIndex1, long ledgerId2, long entryId2, long partitionIndex2, long batchIndex2 )

gmethvin · 2021-03-19T22:14:54Z

@eolivelli That sounds workable. I'd assume we'd want to do the same for compareTo as well?

eolivelli · 2021-03-20T06:14:03Z

Sure

315157973 · 2021-03-20T16:36:53Z

in order to implement hashCode you could add a static utility method that receives all of the relevant fields:
public static int computeMessageIdHashCode(long ledgerId, long entryId, long partitionIndex, long batchIndex)

batchIndex is an attribute that only BatchMessageId has, and it doesn't seem appropriate to put it in the top MessageId.

I don’t know much about the historical reasons, why is there such a parameter as NO_BATCH in BatchMessageIdImpl

There is a special treatment for NO_BATCH in equals. When batchIndex=-1 (NO_BATCH), only ledgerId, entryId and partitionIndex need to be equal.

Can the HashCode also be specially processed? When batchIndex=-1, batchIndex is not allowed to participate in the calculation of hashcode

@eolivelli

eolivelli · 2021-03-20T16:43:33Z

The constraint is that if two objects are 'equals' to each other they have the same hash code.

315157973 · 2021-03-20T16:46:45Z

The constraint is that if two objects are 'equals' to each other they have the same hash code.

If we do special processing in hashCode as in the euqals method, when batchIndex=-1, batchIndex is not allowed to participate in the calculation of hashcode, so the hashCode is equal

The problem now is that special processing is done in equals, but not in hashcode:

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java

Line 109 in 6089292

&& batchIndex == NO_BATCH;

gmethvin · 2021-03-23T23:50:28Z

@315157973 I'm also unsure about why we have NO_BATCH. Can an unbatched message can't be treated like a batched message with an index of 0?

315157973 · 2021-03-24T14:58:16Z

@315157973 I'm also unsure about why we have NO_BATCH. Can an unbatched message can't be treated like a batched message with an index of 0?

The current hashcode is like this
return (int) (31 * (ledgerId + 31 * entryId) + (31 * (long) partitionIndex) + batchIndex);

If it is consistent with the logic of equal, it should be:

if(batchIndex == NO_BATCH){
return (int) (31 * (ledgerId + 31 * entryId) + (31 * (long) partitionIndex));
}
return (int) (31 * (ledgerId + 31 * entryId) + (31 * (long) partitionIndex) + batchIndex);

cc @eolivelli

eolivelli · 2021-03-24T15:00:49Z

@315157973 works for me.
Let's add tests that cover all edge cases

gmethvin · 2021-03-24T21:01:17Z

@315157973 Let me clarify what I'm asking.

As you mentioned, the current BatchMessageIdImpl#equals takes into account all fields including the batchIndex:

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java

Line 109 in 6089292

&& batchIndex == NO_BATCH;

. If there is no batchIndex it is assumed to be NO_BATCH (-1).

And BatchMessageIdImpl#hashCode is:

return (int) (31 * (ledgerId + 31 * entryId) + (31 * (long) partitionIndex) + batchIndex);

So for BatchMessageIdImpl, it does appear consistent with equals, as it takes into account the batchIndex.

The inconsistency is that MessageIdImpl#hashCode does not take into account the batchIndex:

return (int) (31 * (ledgerId + 31 * entryId) + partitionIndex);

This can be fixed by using the the same implementation as in BatchMessageIdImpl#hashCode and assuming batchIndex = NO_BATCH, like I have done in this pull request.

I think you are suggesting changing BatchMessageIdImpl#hashCode to:

if(batchIndex == NO_BATCH){
   return (int) (31 * (ledgerId + 31 * entryId) + (31 * (long) partitionIndex));
}
return (int) (31 * (ledgerId + 31 * entryId) + (31 * (long) partitionIndex) + batchIndex);

Which would only be necessary if equals treats batchIndex = NO_BATCH as equivalent to batchIndex = 0, which it does not currently. But should it?

Or are you suggesting only treating batchIndex = NO_BATCH like batchIndex = 0 only for the purposes of hashCode?

gmethvin · 2021-03-24T21:40:20Z

I talked to @sijie and he said that batchIndex = 0 and batchIndex = NO_BATCH should not be treated as not equal, so it should be fine for them to be both unequal and have unequal hash codes.

gmethvin · 2021-03-25T00:03:37Z

@315157973 @eolivelli I updated to delegate to the static method.

I think I understand what you're saying now. Your change to BatchMessageIdImpl#hashCode works if you keep the existing code in MessageIdImpl#hashCode. The problem is now you have the logic in two different places, which I'd like to avoid, since that's probably why we ended up with this inconsistency to begin with. Having the logic in one place makes the code a lot easier to understand and reduces the chance of future breakage.

315157973 · 2021-03-26T02:45:25Z

works for me

gmethvin · 2021-03-26T03:47:24Z

Thanks @315157973. I added some tests for some of the edge cases, but I'd appreciate feedback on any other tests I can add.

codelipenghui

LGTM

eolivelli

LGTM

eolivelli · 2021-03-29T06:47:39Z

pulsar-client/src/test/java/org/apache/pulsar/client/impl/MessageIdCompareToTest.java

@@ -73,7 +73,7 @@ public void testGreaterThan() {
        assertTrue(batchMessageId2.compareTo(batchMessageId3) > 0, "Expected to be greater than");
        assertTrue(batchMessageId2.compareTo(batchMessageId4) > 0, "Expected to be greater than");
        assertTrue(batchMessageId2.compareTo(batchMessageId5) > 0, "Expected to be greater than");
-        assertTrue(batchMessageId3.compareTo(batchMessageId4) > 0, "Expected to be greater than");
+        assertTrue(batchMessageId4.compareTo(batchMessageId3) > 0, "Expected to be greater than");


interesting catch

eolivelli · 2021-03-29T06:50:41Z

@merlimat can you take a final look ?

315157973

Good job!

codelipenghui · 2021-03-29T14:36:14Z

@merlimat Could you please help review it again

golden-yang

Good job! LGTM

merlimat

👍

eolivelli · 2021-04-29T07:57:44Z

This patch is too dangerous for 2.7.2 IMHO, I have removed the release/2.7.2 label
@codelipenghui

merlimat reviewed Feb 3, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java Outdated Show resolved Hide resolved

sijie assigned gmethvin Feb 3, 2021

sijie added this to the 2.8.0 milestone Feb 3, 2021

sijie added area/client java labels Feb 3, 2021

gmethvin force-pushed the fix-messageId-equals branch 4 times, most recently from 57b7ddb to 5395426 Compare February 4, 2021 00:13

codelipenghui requested review from sijie, 315157973, hangc0276, jiazhai and zymap March 2, 2021 14:15

codelipenghui reviewed Mar 2, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java Outdated Show resolved Hide resolved

315157973 reviewed Mar 2, 2021

View reviewed changes

pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java Show resolved Hide resolved

gmethvin force-pushed the fix-messageId-equals branch from 5395426 to 92db163 Compare March 24, 2021 21:19

gmethvin force-pushed the fix-messageId-equals branch from 92db163 to cb72764 Compare March 24, 2021 21:47

gmethvin force-pushed the fix-messageId-equals branch 2 times, most recently from b43cf81 to f946510 Compare March 25, 2021 00:50

Fix inconsistent equals and hashCode for MessageIds

c77db43

gmethvin force-pushed the fix-messageId-equals branch from f946510 to c77db43 Compare March 25, 2021 01:11

codelipenghui approved these changes Mar 29, 2021

View reviewed changes

eolivelli approved these changes Mar 29, 2021

View reviewed changes

eolivelli requested a review from 315157973 March 29, 2021 06:50

315157973 approved these changes Mar 29, 2021

View reviewed changes

codelipenghui added the release/2.7.2 label Mar 29, 2021

golden-yang approved these changes Apr 6, 2021

View reviewed changes

merlimat approved these changes Apr 21, 2021

View reviewed changes

merlimat merged commit f56ae72 into apache:master Apr 21, 2021

gmethvin deleted the fix-messageId-equals branch April 21, 2021 22:58

eolivelli removed the release/2.7.2 label Apr 29, 2021

BewareMyPower mentioned this pull request May 14, 2021

Fix NPE when ACK grouping tracker checks duplicated message id #10586

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 9438][pulsar-client] Fix inconsistent equals and hashCode for MessageIds #9440

[Issue 9438][pulsar-client] Fix inconsistent equals and hashCode for MessageIds #9440

gmethvin commented Feb 3, 2021 •

edited

Loading

gmethvin commented Feb 4, 2021

sijie commented Feb 8, 2021

gmethvin commented Mar 18, 2021

eolivelli commented Mar 19, 2021

gmethvin commented Mar 19, 2021

eolivelli commented Mar 20, 2021

315157973 commented Mar 20, 2021 •

edited

Loading

eolivelli commented Mar 20, 2021

315157973 commented Mar 20, 2021 •

edited

Loading

gmethvin commented Mar 23, 2021

315157973 commented Mar 24, 2021

eolivelli commented Mar 24, 2021

gmethvin commented Mar 24, 2021

gmethvin commented Mar 24, 2021

gmethvin commented Mar 25, 2021

315157973 commented Mar 26, 2021

gmethvin commented Mar 26, 2021

codelipenghui left a comment

eolivelli left a comment

eolivelli Mar 29, 2021

eolivelli commented Mar 29, 2021

315157973 left a comment

codelipenghui commented Mar 29, 2021

golden-yang left a comment

merlimat left a comment

eolivelli commented Apr 29, 2021

[Issue 9438][pulsar-client] Fix inconsistent equals and hashCode for MessageIds #9440

[Issue 9438][pulsar-client] Fix inconsistent equals and hashCode for MessageIds #9440

Conversation

gmethvin commented Feb 3, 2021 • edited Loading

Motivation

Modifications

Verifying this change

gmethvin commented Feb 4, 2021

sijie commented Feb 8, 2021

gmethvin commented Mar 18, 2021

eolivelli commented Mar 19, 2021

gmethvin commented Mar 19, 2021

eolivelli commented Mar 20, 2021

315157973 commented Mar 20, 2021 • edited Loading

eolivelli commented Mar 20, 2021

315157973 commented Mar 20, 2021 • edited Loading

gmethvin commented Mar 23, 2021

315157973 commented Mar 24, 2021

eolivelli commented Mar 24, 2021

gmethvin commented Mar 24, 2021

gmethvin commented Mar 24, 2021

gmethvin commented Mar 25, 2021

315157973 commented Mar 26, 2021

gmethvin commented Mar 26, 2021

codelipenghui left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Mar 29, 2021

Choose a reason for hiding this comment

eolivelli commented Mar 29, 2021

315157973 left a comment

Choose a reason for hiding this comment

codelipenghui commented Mar 29, 2021

golden-yang left a comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

eolivelli commented Apr 29, 2021

gmethvin commented Feb 3, 2021 •

edited

Loading

315157973 commented Mar 20, 2021 •

edited

Loading

315157973 commented Mar 20, 2021 •

edited

Loading