ChenSammi commented on PR #3982: URL: https://github.com/apache/ozone/pull/3982#issuecomment-1347708881
> Hi @ChenSammi I am really sorry about taking this much time to conduct the review, please find one general thought here, and the rest inline. I think the MonitorTask itself is pretty much repetitive, while also the setup of the executorService code added to OM DN and Recon seems to be pretty much similar. > > Wouldn't it be better to move this into a separated class, and provide the details as some functionals? The only difference I spot for the first time is how the code saves the certificate's serial id to it storage. The rest seems to be pretty much the same code. What do you think, can we put it into a separate class where we can setup the background exeutor start it, and we also can generally define the monitoring task implementation? @fapifta , thanks for the review. There are two differences for each component's monitoring task, one is the way to terminate the service, one is how component certificate id is persisted. Let me try if it can be put into the CertificateClient implementation, then every certificate client can monitor it's certificate lifetime internally. But we need some callbacks implemented by component service to provide the certificate ID persist and service shutdown. Given overall there is just a few lines of code in monitoring task, not sure if these type of refactor will look more concise. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
