Galsza commented on code in PR #5163:
URL: https://github.com/apache/ozone/pull/5163#discussion_r1295607013
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##########
@@ -393,20 +398,35 @@ public static void upgradeToSequenceId(SCMMetadataStore
scmMetadataStore)
CONTAINER_ID, sequenceIdTable.get(CONTAINER_ID));
}
- // upgrade root certificate ID
- if (sequenceIdTable.get(ROOT_CERTIFICATE_ID) == null) {
- long largestRootCertId = BigInteger.ONE.longValueExact();
+ upgradeToCertificateSequenceId(scmMetadataStore, false);
+ }
+
+ public static void upgradeToCertificateSequenceId(
+ SCMMetadataStore scmMetadataStore, boolean force) throws IOException {
+ Table<String, Long> sequenceIdTable =
scmMetadataStore.getSequenceIdTable();
+
+ // upgrade certificate ID table
+ if (sequenceIdTable.get(CERTIFICATE_ID) == null || force) {
+ // Start from ID 2.
+ // ID 1 - root certificate, ID 2 - first SCM certificate.
+ long largestCertId = BigInteger.ONE.add(BigInteger.ONE).longValueExact();
try (TableIterator<BigInteger,
? extends KeyValue<BigInteger, X509Certificate>> iterator =
scmMetadataStore.getValidSCMCertsTable().iterator()) {
while (iterator.hasNext()) {
X509Certificate cert = iterator.next().getValue();
- if (HASecurityUtils.isSelfSignedCertificate(cert) &&
- HASecurityUtils.isCACertificate(cert)) {
- largestRootCertId =
- Long.max(cert.getSerialNumber().longValueExact(),
- largestRootCertId);
- }
+ largestCertId = Long.max(cert.getSerialNumber().longValueExact(),
Review Comment:
Is it possible here that we have an older certificate that still has an id
out of the long range here?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##########
@@ -393,20 +398,35 @@ public static void upgradeToSequenceId(SCMMetadataStore
scmMetadataStore)
CONTAINER_ID, sequenceIdTable.get(CONTAINER_ID));
}
- // upgrade root certificate ID
- if (sequenceIdTable.get(ROOT_CERTIFICATE_ID) == null) {
- long largestRootCertId = BigInteger.ONE.longValueExact();
+ upgradeToCertificateSequenceId(scmMetadataStore, false);
+ }
+
+ public static void upgradeToCertificateSequenceId(
+ SCMMetadataStore scmMetadataStore, boolean force) throws IOException {
+ Table<String, Long> sequenceIdTable =
scmMetadataStore.getSequenceIdTable();
+
+ // upgrade certificate ID table
+ if (sequenceIdTable.get(CERTIFICATE_ID) == null || force) {
+ // Start from ID 2.
+ // ID 1 - root certificate, ID 2 - first SCM certificate.
+ long largestCertId = BigInteger.ONE.add(BigInteger.ONE).longValueExact();
Review Comment:
This is fine by me so you don't need to change it, but it's a complex way to
calculate the constant 2. With the comment it's already easy to understand what
the 2 constant means and I don't see the additional benefit of seeing how we
arrive at that constant. So I'd just write `long largestCertId = 2L` But either
way is fine by me.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##########
@@ -69,7 +69,12 @@ public class SequenceIdGenerator {
public static final String LOCAL_ID = "localId";
public static final String DEL_TXN_ID = "delTxnId";
public static final String CONTAINER_ID = "containerId";
+
+ // TODO: It can be deleted after several Ozone releases
+ @Deprecated
public static final String ROOT_CERTIFICATE_ID = "rootCertificateId";
+ // certificate ID for all services
Review Comment:
This comment doesn't give too much new information this way, does this mean
that instead of the `ROOT_CERTIFICATE_ID` this is now used for all services?
You could mention it perhaps.
--
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]
