Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[refactor][client] Introduce PulsarApiMessageId to access fields of MessageIdData #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[refactor][client] Introduce PulsarApiMessageId to access fields of MessageIdData #11

wants to merge 1 commit into from

Conversation

BewareMyPower
Copy link
Owner

No description provided.

@BewareMyPower BewareMyPower changed the title Introduce MessageIdDataInterface to avoid unnecessary type cast [enh][client] Introduce MessageIdDataInterface to avoid unnecessary type cast Dec 7, 2022
@BewareMyPower BewareMyPower changed the title [enh][client] Introduce MessageIdDataInterface to avoid unnecessary type cast [improve][client] Introduce MessageIdDataInterface to avoid unnecessary type cast Dec 7, 2022
@BewareMyPower BewareMyPower changed the title [improve][client] Introduce MessageIdDataInterface to avoid unnecessary type cast [improve][client] Introduce MessageIdDataInterface to avoid type cast Dec 7, 2022
@BewareMyPower BewareMyPower force-pushed the bewaremypower/introduce-msg-id-data-wrapper branch 4 times, most recently from ef4ce86 to 1f5c2e9 Compare December 12, 2022 06:52
@BewareMyPower BewareMyPower changed the title [improve][client] Introduce MessageIdDataInterface to avoid type cast [improve][client] Introduce PulsarApiMessageId to access fields of MessageIdData Dec 12, 2022
@BewareMyPower BewareMyPower force-pushed the bewaremypower/introduce-msg-id-data-wrapper branch 2 times, most recently from 4200a62 to ed8933d Compare December 12, 2022 16:16
@BewareMyPower BewareMyPower mentioned this pull request Dec 12, 2022
Closed
4 tasks
@BewareMyPower BewareMyPower force-pushed the bewaremypower/introduce-msg-id-data-wrapper branch from ed8933d to 539b150 Compare December 12, 2022 16:46
@BewareMyPower BewareMyPower changed the title [improve][client] Introduce PulsarApiMessageId to access fields of MessageIdData [refactor][client] Introduce PulsarApiMessageId to access fields of MessageIdData Dec 12, 2022
@BewareMyPower
### Motivation
4029416 
Currently the `MessageId` interface hiddens all fields of the
`MessageIdData` struct defined in `PulsarApi.proto`. It's usually enough
for application users because they don't need to access the fields. But
for client developers and developers of other Pulsar ecosystems (e.g.
the built-in Kafka connector and the Flink connector in another repo),
the `MessageId` interface is too simple and there is no common used
abstraction. We can see many code usages like:

```java
if (msgId instanceof BatchMessageIdImpl) {
    // Do type cast and then access fields like ledger id...
} else if (msgId instanceof MessageIdImpl) {
    // Do type cast and then access fields like ledger id...
    // NOTE: don't put this else if before the previous one because
    // BatchMessageIdImpl is also a MessageIdImpl
} // ...
```

These `MessageId` implementations are used directly. It's a very bad
design because any change to the public APIs of these implementations
could bring breaking changes.

Also, there is a `TopicMessageIdImpl` that each time a
`getInnerMessageId()` method must be used to get the underlying
`MessageId` object, then do the type assertion and cast again. It makes
code unnecessarily complicated.

### Modifications

Introduce the `PulsarApiMessageId` interface into the `pulsar-common`
module. All `MessageId` implementations so far (except `MultiMessageId`)
should extend this interface so we can do the following conversion
safely in client code or other modules:

```java
long ledgerId = ((PulsarApiMessageId) msgId).getLedgerId();
```

Regarding the `ack_set` field, use a `BitSet` instead of the
`BatchMessageAcker` to record if a message in the batch is acknowledged.

Since the `TopicMessageId` is just a proxy of other `MessageId`
implementations, it's stored as key or value in the map directly because
the `compareTo`/`equal`/`hashCode` methods have the same semantics with
the underlying `MessageId`. There is no need to cast the type and call
`getInnerMessageId`.

Remove all other usages and mark the public methods as deprecated to
avoid breaking changes. They could be removed in the next major release.

Add a `CustomMessageIdTest` to verify any valid `MessageId` implementation
works for `seek` and `acknowledge` APIs.
@BewareMyPower BewareMyPower force-pushed the bewaremypower/introduce-msg-id-data-wrapper branch from 539b150 to 4029416 Compare December 13, 2022 16:23
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@BewareMyPower