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/08/31 03:16:57 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #7937: [Issue 7878][broker]Fix NPE when acknowledge messages at the broker side
Technoboy- opened a new pull request #7937:
URL: https://github.com/apache/pulsar/pull/7937
----------------------------------------------------------------
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] lhotari commented on pull request #7937: [Issue 7878][broker]Fix NPE when acknowledge messages at the broker side
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #7937:
URL: https://github.com/apache/pulsar/pull/7937#issuecomment-686925505
@Technoboy- @codelipenghui @jiazhai Hi, I was looking at the fix for the NPE. It looks like the fix would silently by-pass any operations when dispatcher is null. Wouldn't skipping the operations cause issues in consistency? Is it ok to skip the operations? I guess it is, but I'm just trying to understand Pulsar coding style by following how it's developed. Therefore I'm asking these questions. Perhaps some day I'd be able to contribute too. :)
Just thinking out loud here. I don't mean to be rude.
There are 16 times that `dispatcher != null` is in the code of PersistentSubscription. Why is dispatcher null in the first place? Would there be a need to improve the design, for example have some kind of state machine / model that would help organize the code in a better way? It seems that the design isn't optimal if each time you call a method on dispatcher, you would have to do a null check. I'd assume that some kind of proxy pattern would encapsulate things in a better way.
(Another unrelated observation is that there are a lot of synchronized methods in the PersistentSubscription class, however I guess there isn't a lockfree design in Pulsar in general.)
----------------------------------------------------------------
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 #7937: [Issue 7878][broker]Fix NPE when acknowledge messages at the broker side
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #7937:
URL: https://github.com/apache/pulsar/pull/7937#issuecomment-686985367
@lhotari Thanks for the comment.
----------------------------------------------------------------
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 merged pull request #7937: [Issue 7878][broker]Fix NPE when acknowledge messages at the broker side
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #7937:
URL: https://github.com/apache/pulsar/pull/7937
----------------------------------------------------------------
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 edited a comment on pull request #7937: [Issue 7878][broker]Fix NPE when acknowledge messages at the broker side
Posted by GitBox <gi...@apache.org>.
codelipenghui edited a comment on pull request #7937:
URL: https://github.com/apache/pulsar/pull/7937#issuecomment-686985367
@lhotari Thanks for the comment. This will not cause issues in consistency, this PR only add check for the acknowledge method. Currently, the acknowledgment is in a `try best` way.
> for example have some kind of state machine / model that would help organize the code in a better way?
Sounds good. If you are interested in this part, feel free to push forward.
> Another unrelated observation is that there are a lot of synchronized methods in the PersistentSubscription class, however I guess there isn't a lockfree design in Pulsar in general.
Yes, maybe this is the part that can be improved. Some seem to be unnecessary.
----------------------------------------------------------------
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