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 2020/12/31 12:15:08 UTC
[GitHub] [pulsar] Shawyeok opened a new pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Shawyeok opened a new pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
### Motivation
Call synchronized method *producer.send* to redeliver unacknowledged message in a TimerTask is dangerous, it may block timer thread forever in some circumstances. e.g. broker crash, producer state is *Connecting*
### Modifications
Call *sendAsync* method instead of *send* method.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is not covered by unit test yet.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (don't know)
### Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not applicable)
- If a feature is not applicable for documentation, explain why? (not applicable)
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation (not applicable)
----------------------------------------------------------------
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] sijie commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-756585221
----------------------------------------------------------------
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] codelipenghui closed pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102
----------------------------------------------------------------
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] zymap commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-785070866
ping @codelipenghui @Shawyeok
move this to the next release.
----------------------------------------------------------------
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] Shawyeok commented on a change in pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on a change in pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#discussion_r550747491
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -107,6 +60,22 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
+import java.util.*;
Review comment:
OK, do we have the exported settings file of code style for IDEA?
----------------------------------------------------------------
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] codelipenghui commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-788549945
https://github.com/apache/pulsar/pull/9552 fixed this problem
----------------------------------------------------------------
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] Shawyeok commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-753931698
/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] codelipenghui commented on a change in pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#discussion_r550955452
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -107,6 +60,22 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
+import java.util.*;
Review comment:
There is no setting file for IDEA yet, You can manually set the idea first, we are advance checkstyle and spotbugs for Pulsar, the IDEA settings also be considered.
----------------------------------------------------------------
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] codelipenghui commented on a change in pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#discussion_r550955691
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -1843,18 +1812,29 @@ private boolean processPossibleToDLQ(MessageIdImpl messageId) {
}
}
if (deadLetterProducer != null) {
- try {
- for (MessageImpl<T> message : deadLetterMessages) {
- deadLetterProducer.newMessage()
- .value(message.getValue())
- .properties(message.getProperties())
- .send();
- }
- acknowledge(messageId);
- return true;
- } catch (Exception e) {
- log.error("Send to dead letter topic exception with topic: {}, messageId: {}", deadLetterProducer.getTopic(), messageId, e);
+ for (MessageImpl<T> message : deadLetterMessages) {
+ deadLetterProducer.newMessage()
+ .value(message.getValue())
+ .properties(message.getProperties())
+ .sendAsync()
+ .whenComplete((msgId, ex) -> {
+ if (ex != null) {
+ log.error("Send to dead letter topic exception with topic: {}, messageId: {}",
+ deadLetterProducer.getTopic(), messageId, ex);
+ return;
+ }
+
+ log.debug("Redeliver dead letter message topic: {}, messageId: {} to dead letter topic with topic: {}, messageId: {}",
+ message.getTopicName(), messageId, deadLetterProducer.getTopic(), msgId);
+ try {
+ acknowledge(messageId);
+ } catch (Exception e) {
+ log.error("Acknowledge dead letter message exception with topic: {}, messageId: {}",
+ message.getTopicName(), messageId);
+ }
+ });
}
+ return true;
Review comment:
It's not a safe operation since the client might crash before the next retry.
> but make the method processPossibleToDLQ asynchronous will bring many other changes to fit async.
Yes, this will introduce more changes, but this is not to introduce some irrelevant code. Change the `processPossibleToDLQ` asynchronously will avoid the complexity of solving this problem from the root cause
----------------------------------------------------------------
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] sijie commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-757613695
PIng @codelipenghui
----------------------------------------------------------------
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] codelipenghui commented on a change in pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#discussion_r550955691
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -1843,18 +1812,29 @@ private boolean processPossibleToDLQ(MessageIdImpl messageId) {
}
}
if (deadLetterProducer != null) {
- try {
- for (MessageImpl<T> message : deadLetterMessages) {
- deadLetterProducer.newMessage()
- .value(message.getValue())
- .properties(message.getProperties())
- .send();
- }
- acknowledge(messageId);
- return true;
- } catch (Exception e) {
- log.error("Send to dead letter topic exception with topic: {}, messageId: {}", deadLetterProducer.getTopic(), messageId, e);
+ for (MessageImpl<T> message : deadLetterMessages) {
+ deadLetterProducer.newMessage()
+ .value(message.getValue())
+ .properties(message.getProperties())
+ .sendAsync()
+ .whenComplete((msgId, ex) -> {
+ if (ex != null) {
+ log.error("Send to dead letter topic exception with topic: {}, messageId: {}",
+ deadLetterProducer.getTopic(), messageId, ex);
+ return;
+ }
+
+ log.debug("Redeliver dead letter message topic: {}, messageId: {} to dead letter topic with topic: {}, messageId: {}",
+ message.getTopicName(), messageId, deadLetterProducer.getTopic(), msgId);
+ try {
+ acknowledge(messageId);
+ } catch (Exception e) {
+ log.error("Acknowledge dead letter message exception with topic: {}, messageId: {}",
+ message.getTopicName(), messageId);
+ }
+ });
}
+ return true;
Review comment:
It's not a safe operation since the client might crash before the next retry.
----------------------------------------------------------------
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] Shawyeok commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-753606384
> Could you please provide more details about the problem such as the thread dump and the root cause.
>
> By the way, it's better to create a new branch to send out the PR, for more details, you can refer to http://pulsar.apache.org/en/contributing/
See details in #9109
----------------------------------------------------------------
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] codelipenghui commented on a change in pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#discussion_r550479879
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -1843,18 +1812,29 @@ private boolean processPossibleToDLQ(MessageIdImpl messageId) {
}
}
if (deadLetterProducer != null) {
- try {
- for (MessageImpl<T> message : deadLetterMessages) {
- deadLetterProducer.newMessage()
- .value(message.getValue())
- .properties(message.getProperties())
- .send();
- }
- acknowledge(messageId);
- return true;
- } catch (Exception e) {
- log.error("Send to dead letter topic exception with topic: {}, messageId: {}", deadLetterProducer.getTopic(), messageId, e);
+ for (MessageImpl<T> message : deadLetterMessages) {
+ deadLetterProducer.newMessage()
+ .value(message.getValue())
+ .properties(message.getProperties())
+ .sendAsync()
+ .whenComplete((msgId, ex) -> {
+ if (ex != null) {
+ log.error("Send to dead letter topic exception with topic: {}, messageId: {}",
+ deadLetterProducer.getTopic(), messageId, ex);
+ return;
+ }
+
+ log.debug("Redeliver dead letter message topic: {}, messageId: {} to dead letter topic with topic: {}, messageId: {}",
+ message.getTopicName(), messageId, deadLetterProducer.getTopic(), msgId);
+ try {
+ acknowledge(messageId);
+ } catch (Exception e) {
+ log.error("Acknowledge dead letter message exception with topic: {}, messageId: {}",
+ message.getTopicName(), messageId);
+ }
+ });
}
+ return true;
Review comment:
You can't return true here because this will always return true even if the message write to the dead letter topic fails. This is used to filter out the messages that should not be delivered.
I think maybe you should change the method `processPossibleToDLQ` to `processPossibleToDLQAsync`.
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -107,6 +60,22 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
+import java.util.*;
Review comment:
Please avoid use import .*
----------------------------------------------------------------
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] zymap commented on pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#issuecomment-784185368
Hi, @Shawyeok. Could you please rebase the code? There have conflicts that need to resolve.
@codelipenghui Please take a look at this.
----------------------------------------------------------------
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] Shawyeok commented on a change in pull request #9102: [pulsar-client] Fix redeliverUnacknowledgedMessages may block timer thread cause serious problem
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on a change in pull request #9102:
URL: https://github.com/apache/pulsar/pull/9102#discussion_r550747420
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -1843,18 +1812,29 @@ private boolean processPossibleToDLQ(MessageIdImpl messageId) {
}
}
if (deadLetterProducer != null) {
- try {
- for (MessageImpl<T> message : deadLetterMessages) {
- deadLetterProducer.newMessage()
- .value(message.getValue())
- .properties(message.getProperties())
- .send();
- }
- acknowledge(messageId);
- return true;
- } catch (Exception e) {
- log.error("Send to dead letter topic exception with topic: {}, messageId: {}", deadLetterProducer.getTopic(), messageId, e);
+ for (MessageImpl<T> message : deadLetterMessages) {
+ deadLetterProducer.newMessage()
+ .value(message.getValue())
+ .properties(message.getProperties())
+ .sendAsync()
+ .whenComplete((msgId, ex) -> {
+ if (ex != null) {
+ log.error("Send to dead letter topic exception with topic: {}, messageId: {}",
+ deadLetterProducer.getTopic(), messageId, ex);
+ return;
+ }
+
+ log.debug("Redeliver dead letter message topic: {}, messageId: {} to dead letter topic with topic: {}, messageId: {}",
+ message.getTopicName(), messageId, deadLetterProducer.getTopic(), msgId);
+ try {
+ acknowledge(messageId);
+ } catch (Exception e) {
+ log.error("Acknowledge dead letter message exception with topic: {}, messageId: {}",
+ message.getTopicName(), messageId);
+ }
+ });
}
+ return true;
Review comment:
Right, can't simply return true here, but make the method `processPossibleToDLQ` asynchronous will bring many other changes to fit async.
https://github.com/apache/pulsar/blob/7a9ec066e1cc3ca918240776c4c9f805b0a92e96/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L1787-L1794
https://github.com/apache/pulsar/blob/7a9ec066e1cc3ca918240776c4c9f805b0a92e96/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L701-L703
How about re-put message into the `UnAckedMessageTracker` when sent failed? So these messages will be retry in later tick.
----------------------------------------------------------------
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