gmethvin commented on pull request #9440: URL: https://github.com/apache/pulsar/pull/9440#issuecomment-806183484
@315157973 Let me clarify what I'm asking. As you mentioned, the current `BatchMessageIdImpl#equals` takes into account all fields including the `batchIndex`: https://github.com/apache/pulsar/blob/608929227824fe4303f46aa432e42af77bcbf625/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L109. If there is no `batchIndex` it is assumed to be `NO_BATCH` (`-1`). And `BatchMessageIdImpl#hashCode` is: ```java 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`: ```java 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: ```java 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`? -- 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. For queries about this service, please contact Infrastructure at: [email protected]
