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/09/29 09:50:53 UTC
[GitHub] [pulsar] lhotari opened a new pull request #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
lhotari opened a new pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160
Master Issue: #8138
### Motivation
The issue #8138 is about multiple memory leaks happening when using the Pulsar Java Client Reader API.
One of the leaks was previously fixed by #8149 .
This PR fixes two remaining ConsumerImpl memory leak issues:
- leaks of ConsumerImpl instances via active timers (unAckedMessageTracker, acknowledgmentsGroupingTracker, stats)
- leaks of ConsumerImpl instances via ClientCnx when the consumer gets closed by the Broker and the consumer picks a new connection to use
### Modifications
- modifications to fix the leaks of ConsumerImpl via active timers:
- calls to "closeConsumerTasks" were added in the locations where they were missing
- modifications to fix the leaks of ConsumerImpl via ClientCnx
- a solution was added to track to which ClientCnx the ConsumerImpl has been registered. When the ConsumerImpl gets registered to a new ClientCnx, the same instance is unregistered from the previous ClientCnx it was previously registered to.
### Verifying this change
- this PR doesn't include new tests. There is a manual test in https://github.com/lhotari/pulsar/commit/aff700ccd0a7f0264cb3de027ac44eab6a65a258 that has been used to reproduce the memory leak and to verify that no ConsumerImpl references are leaked after the fix has been applied.
----------------------------------------------------------------
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] jiazhai merged pull request #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
Posted by GitBox <gi...@apache.org>.
jiazhai merged pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160
----------------------------------------------------------------
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 #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160#issuecomment-701173062
The CI checks have passed now. @jiazhai Would you be able to review and provide feedback? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160#issuecomment-701137610
/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] lhotari closed pull request #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160
----------------------------------------------------------------
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 #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160#issuecomment-700992920
I have revisited the original PR I made earlier today. The changes are now ready for review.
It seems that the current model used to handle state and state transitions is error prone. The code gets duplicated in several locations and it's hard to be sure if it's fine to unify the behavior in all cases.
Perhaps a model where the state transition triggers "side-effects" would be a better way to implement consistent behavior? For example, when the state changes to "Closed", certain "side-effects" would be triggered.
I don't have test coverage for the changes I made. It seems that it would be possible to model consumer's state machine and test that it behaves in the correct way. However the barrier for such improvements is fairly large. :)
----------------------------------------------------------------
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 #8160: [Issue 8138][pulsar-client] Fix ConsumerImpl memory leaks
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #8160:
URL: https://github.com/apache/pulsar/pull/8160#issuecomment-700640181
/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