You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2023/04/07 11:11:51 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request, #20040: [improve][client] PIP-224: Add getLastMessageIds API
BewareMyPower opened a new pull request, #20040:
URL: https://github.com/apache/pulsar/pull/20040
Master Issue: https://github.com/apache/pulsar/issues/18616
Fixes https://github.com/apache/pulsar/issues/4940
NOTE: This implementation is different from the original design of PIP-224 that the method name is `getLastMessageIds` instead of `getLastTopicMessageId`.
### Motivation
When a multi-topics consumer calls `getLastMessageId`, a `MultiMessageIdImpl` instance will be returned. It contains a map of the topic name and the latest message id of the topic. However, the `MultiMessageIdImpl` cannot be used in any place of the API that accepts a `MessageId` because all methods of the `MessageId` interface are not implemented, including `compareTo` and `toByteArray`.
Therefore, users cannot do anything on such a `MessageId` implementation except casting `MessageId` to `MultiMessageIdImpl` and get the internal map.
### Modifications
- Throw an exception when calling `getLastMessageId` on a multi-topics consumer instead of returning a `MultiMessageIdImpl`.
- Remove the `MultiMessageIdImpl` implementation and its related tests.
- Add the `getLastMessageIds` methods to `Consumer`. It returns a list of `TopicMessageId` instances, each of them represents the last message id of the owner topic.
- Mark the `getLastMessageId` API as deprecated.
### Verifications
- Modify the `TopicsConsumerImplTest#testGetLastMessageId` to test the `getLastMessageIds` for a multi-topics consumer.
- Modify the `TopicReaderTest#testHasMessageAvailable` to test the `getLastMessageIds` for a single topic consumer.
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [x] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [x] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/BewareMyPower/pulsar/pull/25
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#discussion_r1161606167
##########
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Consumer.java:
##########
@@ -536,19 +537,38 @@ CompletableFuture<Void> reconsumeLaterCumulativeAsync(Message<?> message,
CompletableFuture<Void> seekAsync(long timestamp);
/**
- * Get the last message id available for consume.
+ * Get the last message id of the topic subscribed.
*
- * @return the last message id.
+ * @return the last message id of the topic subscribed
+ * @throws PulsarClientException if multiple topics or partitioned topics are subscribed or failed because of a
+ * network issue
+ * NOTE: Use {@link Consumer#getLastMessageIds()} instead.
*/
+ @Deprecated
MessageId getLastMessageId() throws PulsarClientException;
/**
- * Get the last message id available for consume.
- *
- * @return a future that can be used to track the completion of the operation.
+ * The asynchronous version of {@link Consumer#getLastMessageId()}.
+ * NOTE: Use {@link Consumer#getLastMessageIdsAsync()} instead.
*/
+ @Deprecated
CompletableFuture<MessageId> getLastMessageIdAsync();
+ /**
+ * Get all the last message id of the topics the consumer subscribed.
+ *
+ * @return the list of TopicMessageId instances of all the topics that the consumer subscribed
+ * @throws PulsarClientException if failed to get last message id.
+ * @apiNote It's guaranteed that the owner topic of each TopicMessageId in the returned list is different from owner
+ * topics of other TopicMessageId instances
+ */
+ List<TopicMessageId> getLastMessageIds() throws PulsarClientException;
Review Comment:
Please also update the proposal according to the new method name.
##########
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Consumer.java:
##########
@@ -536,19 +537,38 @@ CompletableFuture<Void> reconsumeLaterCumulativeAsync(Message<?> message,
CompletableFuture<Void> seekAsync(long timestamp);
/**
- * Get the last message id available for consume.
+ * Get the last message id of the topic subscribed.
*
- * @return the last message id.
+ * @return the last message id of the topic subscribed
+ * @throws PulsarClientException if multiple topics or partitioned topics are subscribed or failed because of a
+ * network issue
+ * NOTE: Use {@link Consumer#getLastMessageIds()} instead.
*/
+ @Deprecated
Review Comment:
If we mark the API as deprecated, we should not change the behavior of the API. The `@Deprecated` annotation helped us to tell users the API will be removed in the future, but not now. Users can migrate to the new API after upgrading to the new version without any compatibility issues. We should mark it as `@Deprecated` first and remove it in the future.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#discussion_r1161682487
##########
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Consumer.java:
##########
@@ -536,19 +537,38 @@ CompletableFuture<Void> reconsumeLaterCumulativeAsync(Message<?> message,
CompletableFuture<Void> seekAsync(long timestamp);
/**
- * Get the last message id available for consume.
+ * Get the last message id of the topic subscribed.
*
- * @return the last message id.
+ * @return the last message id of the topic subscribed
+ * @throws PulsarClientException if multiple topics or partitioned topics are subscribed or failed because of a
+ * network issue
+ * NOTE: Use {@link Consumer#getLastMessageIds()} instead.
*/
+ @Deprecated
Review Comment:
I think we should keep the test until we remove the API. Otherwise, we might introduce breaking changes to the deprecated API in the subsequent releases, maybe a patch release of the LTS version. The deprecated API should also be under the guarantee until it is removed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#discussion_r1161609338
##########
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Consumer.java:
##########
@@ -536,19 +537,38 @@ CompletableFuture<Void> reconsumeLaterCumulativeAsync(Message<?> message,
CompletableFuture<Void> seekAsync(long timestamp);
/**
- * Get the last message id available for consume.
+ * Get the last message id of the topic subscribed.
*
- * @return the last message id.
+ * @return the last message id of the topic subscribed
+ * @throws PulsarClientException if multiple topics or partitioned topics are subscribed or failed because of a
+ * network issue
+ * NOTE: Use {@link Consumer#getLastMessageIds()} instead.
*/
+ @Deprecated
MessageId getLastMessageId() throws PulsarClientException;
/**
- * Get the last message id available for consume.
- *
- * @return a future that can be used to track the completion of the operation.
+ * The asynchronous version of {@link Consumer#getLastMessageId()}.
+ * NOTE: Use {@link Consumer#getLastMessageIdsAsync()} instead.
*/
+ @Deprecated
CompletableFuture<MessageId> getLastMessageIdAsync();
+ /**
+ * Get all the last message id of the topics the consumer subscribed.
+ *
+ * @return the list of TopicMessageId instances of all the topics that the consumer subscribed
+ * @throws PulsarClientException if failed to get last message id.
+ * @apiNote It's guaranteed that the owner topic of each TopicMessageId in the returned list is different from owner
+ * topics of other TopicMessageId instances
+ */
+ List<TopicMessageId> getLastMessageIds() throws PulsarClientException;
Review Comment:
Yeah, I will update it after this PR is merged. Because when the proposal was approved, the method name is not same as it is in this PR. I think we need to let reviewers know the change since the time point when PIP passed the vote.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #20040:
URL: https://github.com/apache/pulsar/pull/20040
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#discussion_r1161624425
##########
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Consumer.java:
##########
@@ -536,19 +537,38 @@ CompletableFuture<Void> reconsumeLaterCumulativeAsync(Message<?> message,
CompletableFuture<Void> seekAsync(long timestamp);
/**
- * Get the last message id available for consume.
+ * Get the last message id of the topic subscribed.
*
- * @return the last message id.
+ * @return the last message id of the topic subscribed
+ * @throws PulsarClientException if multiple topics or partitioned topics are subscribed or failed because of a
+ * network issue
+ * NOTE: Use {@link Consumer#getLastMessageIds()} instead.
*/
+ @Deprecated
Review Comment:
I restored the behaviour now. And the `MultiMessageIdImpl` related tests are still removed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#issuecomment-1502445812
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] shibd commented on a diff in pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "shibd (via GitHub)" <gi...@apache.org>.
shibd commented on code in PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#discussion_r1161756544
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/TopicsConsumerImplTest.java:
##########
@@ -1170,6 +1193,20 @@ public void testGetLastMessageId() throws Exception {
}
});
+ msgIds = consumer.getLastMessageIds();
+ assertEquals(msgIds.size(), 6);
+ assertEquals(msgIds.stream().map(TopicMessageId::getOwnerTopic).collect(Collectors.toSet()), topics);
+ for (TopicMessageId msgId : msgIds) {
+ int numMessages = (int) ((MessageIdAdv) msgId).getEntryId() + 1;
+ if (msgId.getOwnerTopic().equals(topicName1)) {
+ assertEquals(numMessages, totalMessages * 2);
+ } else if (msgId.getOwnerTopic().startsWith(topicName2)) {
+ assertEquals(numMessages, totalMessages / 2 * 2);
Review Comment:
```suggestion
assertEquals(numMessages, totalMessages);
```
It is best to keep the same way as before:
https://github.com/apache/pulsar/pull/20040/files#diff-dc5cb0bbc25d4856fb9e07ace3e45dc27f4da5ac208cf4ef9a885e08416a9eb9R1187-R1192
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #20040: [improve][client] PIP-224: Add getLastMessageIds API
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #20040:
URL: https://github.com/apache/pulsar/pull/20040#discussion_r1161708430
##########
pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Consumer.java:
##########
@@ -536,19 +537,38 @@ CompletableFuture<Void> reconsumeLaterCumulativeAsync(Message<?> message,
CompletableFuture<Void> seekAsync(long timestamp);
/**
- * Get the last message id available for consume.
+ * Get the last message id of the topic subscribed.
*
- * @return the last message id.
+ * @return the last message id of the topic subscribed
+ * @throws PulsarClientException if multiple topics or partitioned topics are subscribed or failed because of a
+ * network issue
+ * NOTE: Use {@link Consumer#getLastMessageIds()} instead.
*/
+ @Deprecated
Review Comment:
It makes sense to me. Let's remove them when we remove the whole `getLastMessageId` API.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org