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: [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]
