BewareMyPower commented on pull request #8606: URL: https://github.com/apache/pulsar/pull/8606#issuecomment-735897363
Besides what I commented, a PR should not contain more than one tasks. I think > - pulsar-client-cpp Consumer do AcknowledgeCumulative just clean up msgId, not <= msgId in UnAckedMessageTrackerEnabled::removeMessagesTill > - potential crash caused by UnAckedMessageTrackerEnabled's timer(see issue like #8519) are too tasks that are not so much related. Could you push another PR for the second issue? Though the tests may need changes once one of two PRs are merged. And the change of `AckGroupingTrackerEnabled` is totally unrelated to this PR. By the way, currently the timer need no `reset` here because each time `AckGroupingTrackerEnabled::scheduleTimer` is called, the timer will be reset to a new instance. I think it's not a efficient way to schedule timer. The timer instance should be constructed in `start()` and destructed in `close()`. As well as the `UnAckedMessageTrackerEnabled`'s timer. ---------------------------------------------------------------- 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: [email protected]
