You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/03/24 00:27:21 UTC
[GitHub] [pulsar] dlg99 opened a new pull request #10025: Corrected Message 's javadoc to reflect actual situation.
dlg99 opened a new pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025
Fixes #10022
### Motivation
Javadoc is out of sync with actual behavior.
To avoid breaking something just changing the javadoc.
Details at https://github.com/apache/pulsar/issues/10022
### Modifications
changed javadoc
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- The public API: (no) - no but updates the doc for public API
### Documentation
- is a javadoc change
--
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:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r616854325
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -273,17 +273,34 @@ protected void failPendingBatchReceives(ConcurrentLinkedQueue<OpBatchReceive<T>>
abstract protected CompletableFuture<Messages<T>> internalBatchReceiveAsync();
+ private static void validateMessageId(Message<?> message) throws PulsarClientException {
+ if (message == null) {
+ throw new PulsarClientException.InvalidMessageException("Non-null message is required");
+ }
+ if (message.getMessageId() == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
+ private static void validateMessageId(MessageId messageId) throws PulsarClientException {
+ if (messageId == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
@Override
public void acknowledge(Message<?> message) throws PulsarClientException {
+ validateMessageId(message);
try {
acknowledge(message.getMessageId());
- } catch (NullPointerException npe) {
- throw new PulsarClientException.InvalidMessageException(npe.getMessage());
+ } catch (Exception e) {
Review comment:
why this is not a `catch PulsarClientException ` ?
the same applies to other "catch Exception" clauses below
--
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:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10025: Corrected Message 's javadoc to reflect actual situation.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r600193125
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Message.java
##########
@@ -81,7 +81,8 @@
*
* <p>Only messages received from the consumer will have a message id assigned.
*
- * @return the message id null if this message was not received by this client instance
+ * @return the message id
+ * @throws NullPointerException if the message is null (message was not received by this client instance)
Review comment:
In my opinion is it unlikely that a Java program catches an NPE in order to detect this case.
We can remove the null check and release the change in 2.8.0.
I am pretty sure that no one will notice this change
--
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:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #10025: Corrected Message 's javadoc to reflect actual situation.
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r600075236
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Message.java
##########
@@ -81,7 +81,8 @@
*
* <p>Only messages received from the consumer will have a message id assigned.
*
- * @return the message id null if this message was not received by this client instance
+ * @return the message id
+ * @throws NullPointerException if the message is null (message was not received by this client instance)
Review comment:
I think we should really fix the implementation instead of throwing exception
--
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:
users@infra.apache.org
[GitHub] [pulsar] dlg99 commented on a change in pull request #10025: Corrected Message 's javadoc to reflect actual situation.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r600077556
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Message.java
##########
@@ -81,7 +81,8 @@
*
* <p>Only messages received from the consumer will have a message id assigned.
*
- * @return the message id null if this message was not received by this client instance
+ * @return the message id
+ * @throws NullPointerException if the message is null (message was not received by this client instance)
Review comment:
@merlimat I thought about doing that, but that assumes making sure that null check is added where the getMessageId() is used (which is impossible that the code outside of pulsar repo).
I can also simply remove checkNotNull and simply let the NPE happen eventually someplace else but I'd love to have more than one voice for this option.
--
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:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r616863103
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -273,17 +273,34 @@ protected void failPendingBatchReceives(ConcurrentLinkedQueue<OpBatchReceive<T>>
abstract protected CompletableFuture<Messages<T>> internalBatchReceiveAsync();
+ private static void validateMessageId(Message<?> message) throws PulsarClientException {
+ if (message == null) {
+ throw new PulsarClientException.InvalidMessageException("Non-null message is required");
+ }
+ if (message.getMessageId() == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
+ private static void validateMessageId(MessageId messageId) throws PulsarClientException {
+ if (messageId == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
@Override
public void acknowledge(Message<?> message) throws PulsarClientException {
+ validateMessageId(message);
try {
acknowledge(message.getMessageId());
- } catch (NullPointerException npe) {
- throw new PulsarClientException.InvalidMessageException(npe.getMessage());
+ } catch (Exception e) {
Review comment:
in this case
we have to handle InterruptedException and call Thread.currentThread.interrupt();
--
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:
users@infra.apache.org
[GitHub] [pulsar] MarvinCai commented on a change in pull request #10025: Corrected Message 's javadoc to reflect actual situation.
Posted by GitBox <gi...@apache.org>.
MarvinCai commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r600188078
##########
File path: pulsar-client-api/src/main/java/org/apache/pulsar/client/api/Message.java
##########
@@ -81,7 +81,8 @@
*
* <p>Only messages received from the consumer will have a message id assigned.
*
- * @return the message id null if this message was not received by this client instance
+ * @return the message id
+ * @throws NullPointerException if the message is null (message was not received by this client instance)
Review comment:
Seems returning null or throwing NPE is really opinion based. I think it is important not to break client code, e.g. if client code is checking for NPE and not doing null check then change to return null could possible cause NPE in their program.
And I think we should also keep it consistent with other code in terms of returning null or throw.
--
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:
users@infra.apache.org
[GitHub] [pulsar] dlg99 commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r617254061
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -393,29 +425,34 @@ public void reconsumeLaterCumulative(Message<?> message, long delayTime, TimeUni
return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
}
try {
- return doReconsumeLater(message, AckType.Individual, Collections.emptyMap(), delayTime, unit);
- } catch (NullPointerException npe) {
- return FutureUtil.failedFuture(new PulsarClientException.InvalidMessageException(npe.getMessage()));
+ validateMessageId(message);
+ } catch (PulsarClientException e) {
+ return FutureUtil.failedFuture(e);
}
+ return doReconsumeLater(message, AckType.Individual, Collections.emptyMap(), delayTime, unit);
}
@Override
public CompletableFuture<Void> reconsumeLaterAsync(Messages<?> messages, long delayTime, TimeUnit unit) {
- try {
- messages.forEach(message -> reconsumeLaterAsync(message,delayTime, unit));
- return CompletableFuture.completedFuture(null);
- } catch (NullPointerException npe) {
- return FutureUtil.failedFuture(new PulsarClientException.InvalidMessageException(npe.getMessage()));
+ for (Message<?> message: messages) {
Review comment:
fixed
--
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:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r616864169
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -273,17 +273,34 @@ protected void failPendingBatchReceives(ConcurrentLinkedQueue<OpBatchReceive<T>>
abstract protected CompletableFuture<Messages<T>> internalBatchReceiveAsync();
+ private static void validateMessageId(Message<?> message) throws PulsarClientException {
+ if (message == null) {
+ throw new PulsarClientException.InvalidMessageException("Non-null message is required");
+ }
+ if (message.getMessageId() == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
+ private static void validateMessageId(MessageId messageId) throws PulsarClientException {
+ if (messageId == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
@Override
public void acknowledge(Message<?> message) throws PulsarClientException {
+ validateMessageId(message);
try {
acknowledge(message.getMessageId());
- } catch (NullPointerException npe) {
- throw new PulsarClientException.InvalidMessageException(npe.getMessage());
+ } catch (Exception e) {
Review comment:
also if it is possible it is better to catch specific exception type
--
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:
users@infra.apache.org
[GitHub] [pulsar] Jennifer88huang commented on pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
Jennifer88huang commented on pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#issuecomment-809067375
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] dlg99 commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r617054629
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -273,17 +273,34 @@ protected void failPendingBatchReceives(ConcurrentLinkedQueue<OpBatchReceive<T>>
abstract protected CompletableFuture<Messages<T>> internalBatchReceiveAsync();
+ private static void validateMessageId(Message<?> message) throws PulsarClientException {
+ if (message == null) {
+ throw new PulsarClientException.InvalidMessageException("Non-null message is required");
+ }
+ if (message.getMessageId() == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
+ private static void validateMessageId(MessageId messageId) throws PulsarClientException {
+ if (messageId == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
@Override
public void acknowledge(Message<?> message) throws PulsarClientException {
+ validateMessageId(message);
try {
acknowledge(message.getMessageId());
- } catch (NullPointerException npe) {
- throw new PulsarClientException.InvalidMessageException(npe.getMessage());
+ } catch (Exception e) {
Review comment:
@eolivelli fixed
--
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:
users@infra.apache.org
[GitHub] [pulsar] dlg99 commented on pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#issuecomment-824221113
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025
--
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:
users@infra.apache.org
[GitHub] [pulsar] dlg99 commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r616861090
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -273,17 +273,34 @@ protected void failPendingBatchReceives(ConcurrentLinkedQueue<OpBatchReceive<T>>
abstract protected CompletableFuture<Messages<T>> internalBatchReceiveAsync();
+ private static void validateMessageId(Message<?> message) throws PulsarClientException {
+ if (message == null) {
+ throw new PulsarClientException.InvalidMessageException("Non-null message is required");
+ }
+ if (message.getMessageId() == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
+ private static void validateMessageId(MessageId messageId) throws PulsarClientException {
+ if (messageId == null) {
+ throw new PulsarClientException.InvalidMessageException("Cannot handle message with null messageId");
+ }
+ }
+
@Override
public void acknowledge(Message<?> message) throws PulsarClientException {
+ validateMessageId(message);
try {
acknowledge(message.getMessageId());
- } catch (NullPointerException npe) {
- throw new PulsarClientException.InvalidMessageException(npe.getMessage());
+ } catch (Exception e) {
Review comment:
@eolivelli The method throws PulsarClientException / point of this catch it to wrap other exception types into the PulsarClientException. acknowledge() internally gets into acknowledgeAsync().get() which can throw InterruptedException/ExecutionException.
--
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:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #10025: Removed null check in MessageImpl.getMessageId() to match documented behavior.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10025:
URL: https://github.com/apache/pulsar/pull/10025#discussion_r617230010
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java
##########
@@ -393,29 +425,34 @@ public void reconsumeLaterCumulative(Message<?> message, long delayTime, TimeUni
return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
}
try {
- return doReconsumeLater(message, AckType.Individual, Collections.emptyMap(), delayTime, unit);
- } catch (NullPointerException npe) {
- return FutureUtil.failedFuture(new PulsarClientException.InvalidMessageException(npe.getMessage()));
+ validateMessageId(message);
+ } catch (PulsarClientException e) {
+ return FutureUtil.failedFuture(e);
}
+ return doReconsumeLater(message, AckType.Individual, Collections.emptyMap(), delayTime, unit);
}
@Override
public CompletableFuture<Void> reconsumeLaterAsync(Messages<?> messages, long delayTime, TimeUnit unit) {
- try {
- messages.forEach(message -> reconsumeLaterAsync(message,delayTime, unit));
- return CompletableFuture.completedFuture(null);
- } catch (NullPointerException npe) {
- return FutureUtil.failedFuture(new PulsarClientException.InvalidMessageException(npe.getMessage()));
+ for (Message<?> message: messages) {
Review comment:
we should perform validation before actually calling `reconsumeLaterAsync`
otherwise it is possible that `reconsumeLaterAsync` is called only for a part of the messages and not for all.
I suggest to per form the validation loop and then leave as before
```
messages.forEach(message -> reconsumeLaterAsync(message,delayTime, unit));
return CompletableFuture.completedFuture(null);
```
--
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:
users@infra.apache.org