apache / apache/pulsar

PIP-230: Throw exception when MessageIdImpl and BatchMessageIdImpl compare with each other

Open
#18,957 3 comments 0 reactions 0 assignees View on GitHub
Stale type/PIP
Dominant language
Java
Stars
15.3k
Forks
3.8k
Avg merge
1d 14h
Merged PRs (30d)
160

Description

### Motivation

now when `BatchMessageIdImpl` and `MessageIdImpl` with the same`LedgerId` and `EntryId`, one of it compared with the other, the`BatchMessageIdImpl` will always be greater than MessageIdImpl.

**Example:**
batchMessageId = `BatchMessageIdImpl(ledgerId = 1, entryId = 1, batchIndex = 10)`
messageId = `MessageId(ledgerId = 1, entryId = 1)`
batchMessageId.compareTo(messageId) > 0 forever
messageId.compareTo(batchMessageId) < 0 forever
**Compare logic see : **
https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L29
https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L74-L75

https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java#L206-L208

`MessageIdImpl` add this logic: https://github.com/apache/pulsar/pull/6621
`BatchMessageIdImpl` add this logic: https://github.com/apache/pulsar/pull/1285

MessageID is mainly used for seek, rest cursor, Individual ack, and cumulative ack.
now cumulative ack independently fixed this issue: https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L665-L681

if MessageIdImpl(messageId) and BatchMessageIdImpl(batchMessageId) with the same ledgerId and entryId then **batchMessageId⊆messageId**. Therefore, they belong to the inclusion relationship, and they are not of the same type and can be compared. Regardless of the result of the comparison, there is ambiguity and the boundaries are not clear

### Goal

Forbid the comparison between `MessageId` and `BatchMessageId` to make the boundary of `compareTo()` clearer to prevent the wrong usage
### API Changes

https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/BatchMessageIdImpl.java#L74-L75

https://github.com/apache/pulsar/blob/a35670d83b0b3f2fb63d11e4fb222d7f24099c9a/pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java#L206-L208

### Implementation
when `MessageIdImpl.compareTo(BatchMessageIdImpl)`, throw `java.lang.UnsupportedOperationException`
when `BatchMessageIdImpl.compareTo(MessageIdImpl)`, throw `java.lang.UnsupportedOperationException`

MessageIdImpl compareTo() :
```
@Override
public int compareTo(@Nonnull MessageId o) {
if (o instanceof MessageIdImpl) {
if (o instanceof BatchMessageIdImpl) {
throw new UnsupportedOperationException(MessageIdImpl.class.getName()
+ " can't compare with " + BatchMessageIdImpl.class.getName());
}
MessageIdImpl other = (MessageIdImpl) o;
return messageIdCompare(
this.ledgerId, this.entryId, this.partitionIndex, other.ledgerId, other.entryId, other.partitionIndex
);
} else if (o instanceof TopicMessageIdImpl) {
return compareTo(((TopicMessageIdImpl) o).getInnerMessageId());
} else {
throw new UnsupportedOperationException("Unknown MessageId type: " + o.getClass().getName());
}
}
```

BatchMessageIdImpl compareTo() :
```
@Override
public int compareTo(@Nonnull MessageId o) {
if (o instanceof MessageIdImpl) {
if (!(o instanceof BatchMessageIdImpl)) {
throw new UnsupportedOperationException(BatchMessageIdImpl.class.getName()
+ " can't compare with " + MessageIdImpl.class.getName());
}
MessageIdImpl other = (MessageIdImpl) o;
return messageIdCompare(
this.ledgerId, this.entryId, this.partitionIndex, this.batchIndex,
other.ledgerId, other.entryId, other.partitionIndex, ((BatchMessageIdImpl) o).batchIndex
);
} else if (o instanceof TopicMessageIdImpl) {
return compareTo(((TopicMessageIdImpl) o).getInnerMessageId());
} else {
throw new UnsupportedOperationException("Unknown MessageId type: " + o.getClass().getName());
}
}

```
### Alternatives
when `BatchMessageIdImpl` and `MessageIdImpl` with the same`LedgerId` and `EntryId`, `MessageIdImpl` always bigger than `BatchMessageIdImpl`. if use this method, there is still ambiguity, the boundary is not clear

### Anything else?

_No response_

Contributor guide

Open the contributing guide

Research direction

Start with compareTo methods in pulsar-client/src/main/java/org/apache/pulsar/client/impl/MessageIdImpl.java and BatchMessageIdImpl.java, using the linked lines as entry points. Verify that cross-type comparisons throw UnsupportedOperationException in both directions while same-type comparisons retain their existing behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
api
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
44/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.