[GitHub] [ozone] ChenSammi commented on a diff in pull request #4317: HDDS-8030. Cleanup unused/unnecessary code related to CertificateClient

["ChenSammi (via GitHub)" <gi...@apache.org>]

ChenSammi commented on code in PR #4317:
URL: https://github.com/apache/ozone/pull/4317#discussion_r1119636333


##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java:
##########
@@ -162,14 +154,6 @@ public abstract class DefaultCertificateClient implements CertificateClient {
     loadAllCertificates();
   }
 
-  public synchronized void setCertificateId(String certId) {

Review Comment:
   @fapifta , thanks for this cleanup work. 
   
   I would suggest keep this setCertificateId API.  When default certID is changed, loadAllCertificates() need be called to update certificateMap and ca, root ca cert ID too.  This whole process is synchronized to avoid any mix use of old and new data.  
   
   With the remove of  setCertificateId, it's error prone.  In the commit history, loadAllCertificates call is missed in reloadKeyAndCertificate and synchronized key word is missed on signAndStoreCertificate.  And synchronized on signAndStoreCertificate is not appropriate, for signAndStoreCertificate includes the call to SCM to get certificates, any network vibration will block the CertificateClient. 
   
   So my overall suggestion is bring back the setCertificateId function. It may be not necessary be part of CertificateClient interface, we can keep it in DefaultCertificateClient.  
   
   Besides setCertificateId function, other parts look good to me. 
    
   
   



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org