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 2022/05/18 03:40:24 UTC
[GitHub] [pulsar] gvolpe opened a new pull request, #15190: [improve][java-client] Better error message for `reconsumeLater`
gvolpe opened a new pull request, #15190:
URL: https://github.com/apache/pulsar/pull/15190
First time I tried to use this method, I got an unhelpful error. It turns out all I had to do was call `enableRetry(true)` on the consumer. A better error message would have saved me a lot of time :)
--
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] michaeljmarshall commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r854781793
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
Sure, that makes sense, and good catch on my typo.
--
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] michaeljmarshall commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r854762005
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
Instead of asking a question, I think it would be appropriate to return a statement with the name of the specific configuration value. It could be something like:
```suggestion
return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported becuase retryEnabled is false."));
```
--
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] gvolpe commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r854332079
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
@Jason918 @Technoboy- any thoughts?
--
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] Technoboy- commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1135301107
/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] gvolpe commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1107520927
@michaeljmarshall I extracted the same error message into a constant to avoid repetitions (I noticed there's a lot of it regarding error messages), not sure if this is a well-accepted practice but I couldn't stop myself from avoiding DRY 😄
Happy to make any other corrections, though I'm going away for 2 weeks on Monday so I may not be timely with my responses.
--
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] gvolpe commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r852631675
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
@Technoboy- that doesn't sound good. One would say "enable retries" in English. Thus, I proposed "Please set `enableRetry(true)` in your `ConsumerBuilder`".
I think the Pulsar option should be named `enableRetries` instead, but that's a different story...
--
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] github-actions[bot] commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1100902616
@gvolpe:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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] michaeljmarshall closed pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall closed pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
URL: https://github.com/apache/pulsar/pull/15190
--
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] michaeljmarshall commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1129532685
Sorry for the delay @gvolpe. LGTM. I'm going to close and reopen the PR to trigger our updated test suite to run.
--
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] Technoboy- commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r852550866
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
"Please enable retry in ConsumerBuilder to support `reconsumeLater`" ?
--
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] gvolpe commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1128515183
@Jason918 @Technoboy- @HQebupt @michaeljmarshall who can I ping to make something happen on this PR? Thank you.
--
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] gvolpe commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1129698851
Not sure what's the recommended way to fix the style issue: "Line is longer than 120 characters". Any hints? There are so many ways to concatenate strings in Java 😄
--
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] gvolpe commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1129658425
@michaeljmarshall thank you! 🙏🏽
--
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] gvolpe commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r851906809
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
I agree with the second part, but disagree with the first part: we are talking about the method, which is not supported (we can't enable `reconsumeLater`). How about this?
"reconsumeLater method not supported! Please set `enableRetry(true)` in your `ConsumerBuilder`"
--
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] Jason918 commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r851840775
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
It's better to change `supported` to `enabled`. We can provide a direct guide message about how to enable this, like "Please enable retry in ConsumerBuilder"
--
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] gvolpe commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1124881820
@michaeljmarshall have you had a chance to look into the latest commit? Happy to make any other adjustments if required.
--
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] michaeljmarshall commented on pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#issuecomment-1129564007
@gvolpe - looks like you have two checkstyle errors:
```
Error: src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:[104,26] (naming) MemberName: Name 'RECONSUME_LATER_ERROR_MSG' must match pattern '^[a-z][a-zA-Z0-9]*$'.
Error: src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:[105] (sizes) LineLength: Line is longer than 120 characters (found 121).
```
I think you'll need to either make `RECONSUME_LATER_ERROR_MSG` static or make it use camel casing.
--
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] gvolpe commented on a diff in pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
gvolpe commented on code in PR #15190:
URL: https://github.com/apache/pulsar/pull/15190#discussion_r854771483
##########
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java:
##########
@@ -466,7 +466,7 @@ public CompletableFuture<Void> reconsumeLaterAsync(Message<?> message, long dela
public CompletableFuture<Void> reconsumeLaterAsync(
Message<?> message, Map<String, String> customProperties, long delayTime, TimeUnit unit) {
if (!conf.isRetryEnable()) {
- return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not support!"));
+ return FutureUtil.failedFuture(new PulsarClientException("reconsumeLater method not supported! Have you enabled retries in your consumer?"));
Review Comment:
@michaeljmarshall that's a good idea (there's a typo in that commit suggestion, though 😄 ), but I would still add where retryEnabled is false (in the `ConsumerBuilder`), I think that'd make for a better error message.
--
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] Technoboy- merged pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #15190:
URL: https://github.com/apache/pulsar/pull/15190
--
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] Technoboy- closed pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
Posted by GitBox <gi...@apache.org>.
Technoboy- closed pull request #15190: [improve][java-client] Better error message for `reconsumeLater`
URL: https://github.com/apache/pulsar/pull/15190
--
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