Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on PR #10211: URL: https://github.com/apache/ozone/pull/10211#issuecomment-4462186786 > @navinko , filed HDDS-15288 for improving StateManagerImpl. Please see if you are interested working on it. Thanks for the review and merge . Sure will assign myself and start working on it. -- 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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo commented on PR #10211: URL: https://github.com/apache/ozone/pull/10211#issuecomment-4462150801 @navinko , filed HDDS-15288 for improving StateManagerImpl. Please see if you are interested working on it. -- 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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo merged PR #10211: URL: https://github.com/apache/ozone/pull/10211 -- 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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3244078069
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdType.java:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+/**
+ * Represents the sequence ID types managed by {@link SequenceIdGenerator} and
their persisted RocksDB keys.
+ */
+public enum SequenceIdType {
Review Comment:
Thanks @szetszwo
I just updated the changes ,tried incorporating the suggestions.
- used enum constants same as db key
- refactored related code ,test cases and updated required comments .
- tested with existing and updated test cases .ran locally with docker
secure cluster and verified allocation paths and updated under "How was this
patch tested"
- CI build progressing
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3243981759
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdType.java:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+/**
+ * Represents the sequence ID types managed by {@link SequenceIdGenerator} and
their persisted RocksDB keys.
+ */
+public enum SequenceIdType {
Review Comment:
Thanks @szetszwo for the review . Agreed , will update the change shortly.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3236812678
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdType.java:
##
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+/**
+ * Represents the sequence ID types managed by {@link SequenceIdGenerator} and
their persisted RocksDB keys.
+ */
+public enum SequenceIdType {
Review Comment:
@navinko , Thanks for the update!
After some thoughts, .let's simply use the db key as the enum names.
Otherwise, it is confusing to have two different strings.
```java
public enum SequenceIdType {
localId,
delTxnId,
containerId,
/** Certificate ID for all services, including root certificates. */
CertificateId,
/** @deprecated Use {@link #CERTIFICATE_ID} instead. */
@Deprecated
rootCertificateId;
}
```
The change looks good other than that.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on PR #10211: URL: https://github.com/apache/ozone/pull/10211#issuecomment-4427982519 Thanks @szetszwo , reverted all the changes for StateManager and StateManagerImpl and keep using String. -- 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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo commented on PR #10211: URL: https://github.com/apache/ozone/pull/10211#issuecomment-4427257772 @navinko , thanks for the update! Please remove `DB_KEY_MAP` since it is not used except for tests. -- 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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3221877590
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -192,21 +199,21 @@ interface StateManager extends SCMHandler {
* Compare And Swap lastId saved in db from expectedLastId to newLastId.
* If based on Ratis, it will submit a raft client request.
*
- * @param sequenceIdName : name of the sequence id.
+ * @param idType : the type of sequence ID.
* @param expectedLastId : the expected lastId saved in db
* @param newLastId : the new lastId to save in db
* @return : result of the C.A.S.
*/
@Replicate
-Boolean allocateBatch(String sequenceIdName,
+Boolean allocateBatch(SequenceIdType idType,
Review Comment:
Thanks @szetszwo will update the PR as suggested .
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3220211010
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -192,21 +199,21 @@ interface StateManager extends SCMHandler {
* Compare And Swap lastId saved in db from expectedLastId to newLastId.
* If based on Ratis, it will submit a raft client request.
*
- * @param sequenceIdName : name of the sequence id.
+ * @param idType : the type of sequence ID.
* @param expectedLastId : the expected lastId saved in db
* @param newLastId : the new lastId to save in db
* @return : result of the C.A.S.
*/
@Replicate
-Boolean allocateBatch(String sequenceIdName,
+Boolean allocateBatch(SequenceIdType idType,
Review Comment:
We cannot change the parameter class for compatibility. Otherwise, the SCM
in old version cannot talk to the SCM in new version. Let's do it in a
separated JIRA.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on PR #10211: URL: https://github.com/apache/ozone/pull/10211#issuecomment-4421546185 Thanks @szetszwo @sreejasahithi for the review , tried addressing review suggestions . Kindly do review once you get some time. -- 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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219533715
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -128,18 +128,18 @@ public long getNextId(String sequenceIdName) throws
SCMException {
Preconditions.checkArgument(Long.MAX_VALUE - batch.lastId >=
batchSize);
long nextLastId = batch.lastId +
-((sequenceIdName.equals(CERTIFICATE_ID)) ? 1 : batchSize);
+(idType == SequenceIdType.CERTIFICATE_ID ? 1 : batchSize);
-if (stateManager.allocateBatch(sequenceIdName,
+if (stateManager.allocateBatch(idType.getDbKey(),
prevLastId, nextLastId)) {
batch.lastId = nextLastId;
LOG.info("Allocate a batch for {}, change lastId from {} to {}.",
- sequenceIdName, prevLastId, batch.lastId);
+ idType, prevLastId, batch.lastId);
break;
}
// reload lastId from RocksDB.
-batch.lastId = stateManager.getLastId(sequenceIdName);
+batch.lastId = stateManager.getLastId(idType.getDbKey());
Review Comment:
Thanks @sreejasahithi @szetszwo for the suggestion!
I have removed the unused method fromDbKey and DB_KEY_MAP and made all
constant under SequenceIdGenerator private alongwith EnumMap as unmodifiable
and dis suggested changes in test classes.
I've updated code for StateManagerImpl too , the parameters and map keys to
use SequenceIdType instead of Strings. Further , to handle how these new Enums
are serialized across the Ratis and safely deserialized from existing RocksDB
bytes, I implemented the ScmSequenceIdTypeCodec and registered with
ScmCodecFactory. This ensures the Enums translate perfectly into our legacy
byte format .
Also referred existing TestPipelineIDCodec to create new unit test for codec
and verifies the serialization and deserialzation. In the
LegacyStringSequenceIdCodecForTesting class simulated, how Sequence IDs were
serialized before the Enum refactoring.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219533715
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -128,18 +128,18 @@ public long getNextId(String sequenceIdName) throws
SCMException {
Preconditions.checkArgument(Long.MAX_VALUE - batch.lastId >=
batchSize);
long nextLastId = batch.lastId +
-((sequenceIdName.equals(CERTIFICATE_ID)) ? 1 : batchSize);
+(idType == SequenceIdType.CERTIFICATE_ID ? 1 : batchSize);
-if (stateManager.allocateBatch(sequenceIdName,
+if (stateManager.allocateBatch(idType.getDbKey(),
prevLastId, nextLastId)) {
batch.lastId = nextLastId;
LOG.info("Allocate a batch for {}, change lastId from {} to {}.",
- sequenceIdName, prevLastId, batch.lastId);
+ idType, prevLastId, batch.lastId);
break;
}
// reload lastId from RocksDB.
-batch.lastId = stateManager.getLastId(sequenceIdName);
+batch.lastId = stateManager.getLastId(idType.getDbKey());
Review Comment:
Thanks @sreejasahithi @szetszwo for the suggestion!
I have removed the unused method fromDbKey and DB_KEY_MAP and made all
constant under SequenceIdGenerator private alongwith EnumMap as unmodifiable.
I've updated code for StateManagerImpl too , the parameters and map keys to
use SequenceIdType instead of Strings. Further , to handle how these new Enums
are serialized across the Ratis and safely deserialized from existing RocksDB
bytes, I implemented the ScmSequenceIdTypeCodec and registered with
ScmCodecFactory. This ensures the Enums translate perfectly into our legacy
byte format .
Also referred existing TestPipelineIDCodec to create new unit test for codec
and verifies the serialization and deserialzation. In the
LegacyStringSequenceIdCodecForTesting class simulated, how Sequence IDs were
serialized before the Enum refactoring.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219494220
##
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIdType.java:
##
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link SequenceIdType}.
+ */
+public class TestSequenceIdType {
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testStringSyncWithEnumConstants() {
+// Even though we changed to an Enum, these underlying strings are
+// persisted in RocksDB. Do not change them.
+assertEquals("localId", SequenceIdType.LOCAL_ID.getDbKey());
+assertEquals("delTxnId", SequenceIdType.DEL_TXN_ID.getDbKey());
+assertEquals("containerId", SequenceIdType.CONTAINER_ID.getDbKey());
+assertEquals("CertificateId", SequenceIdType.CERTIFICATE_ID.getDbKey());
+assertEquals("rootCertificateId",
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey());
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedStringSyncWithEnumConstants() {
+assertEquals(SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey(),
+SequenceIdGenerator.ROOT_CERTIFICATE_ID);
+ }
+
+ @Test
+ public void testNumberOfEnumConstants() {
+// If a new SequenceIdType is added, this test will fail.
+// This serves as a reminder to the developer to verify RocksDB backward
+// compatibility and update this test class accordingly.
+assertEquals(5, SequenceIdType.values().length);
+ }
Review Comment:
Thanks , it make sense!
removed this one.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219488584
##
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIdType.java:
##
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link SequenceIdType}.
+ */
+public class TestSequenceIdType {
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testStringSyncWithEnumConstants() {
+// Even though we changed to an Enum, these underlying strings are
+// persisted in RocksDB. Do not change them.
+assertEquals("localId", SequenceIdType.LOCAL_ID.getDbKey());
+assertEquals("delTxnId", SequenceIdType.DEL_TXN_ID.getDbKey());
+assertEquals("containerId", SequenceIdType.CONTAINER_ID.getDbKey());
+assertEquals("CertificateId", SequenceIdType.CERTIFICATE_ID.getDbKey());
+assertEquals("rootCertificateId",
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey());
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedStringSyncWithEnumConstants() {
+assertEquals(SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey(),
+SequenceIdGenerator.ROOT_CERTIFICATE_ID);
+ }
+
+ @Test
+ public void testNumberOfEnumConstants() {
+// If a new SequenceIdType is added, this test will fail.
+// This serves as a reminder to the developer to verify RocksDB backward
+// compatibility and update this test class accordingly.
+assertEquals(5, SequenceIdType.values().length);
+ }
+
+ @Test
+ public void testIfNewEnumConstantGetsAdded() {
+Set expectedNames = new HashSet<>(Arrays.asList(
+"LOCAL_ID", "DEL_TXN_ID", "CONTAINER_ID",
+"CERTIFICATE_ID", "ROOT_CERTIFICATE_ID"));
+
+Set actualNames = new HashSet<>();
+for (SequenceIdType type : SequenceIdType.values()) {
+ actualNames.add(type.name());
+}
+
+// Filter exactly what changed to make the failure message extremely clear
+Set added = new HashSet<>(actualNames);
+added.removeAll(expectedNames);
+
+Set removed = new HashSet<>(expectedNames);
+removed.removeAll(actualNames);
+
+assertTrue(added.isEmpty() && removed.isEmpty(),
+() -> "SequenceIdType constants changed!\n" +
+"Unexpectedly Added: " + added + "\n" +
+"Unexpectedly Removed: " + removed + "\n" +
+"ACTION REQUIRED: If this change is intentional, you MUST verify "
+
+"RocksDB backward compatibility and update this test's
expectedNames.");
+ }
+
+ @Test
+ public void testReturnsNullIfEnumConstantNotAvailable() {
+assertNull(SequenceIdType.fromDbKey("unmapped-key-string"));
+assertNull(SequenceIdType.fromDbKey(null));
+ }
Review Comment:
Thanks @sreejasahithi for review, to align with current review comments i
ahve removed the unused method fromDbKey completely.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219311152
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -108,14 +108,14 @@ public StateManager createStateManager(SCMHAManager
scmhaManager,
}
/**
- * @param sequenceIdName : name of the sequenceId
- * @return : next id of this sequenceId.
+ * @param idType : supported sequence ID type
+ * @return next id of this sequence ID.
*/
- public long getNextId(String sequenceIdName) throws SCMException {
+ public long getNextId(SequenceIdType idType) throws SCMException {
lock.lock();
try {
Batch batch = sequenceIdToBatchMap.computeIfAbsent(
- sequenceIdName, key -> new Batch());
+ idType, key -> new Batch());
Review Comment:
Done
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219306300
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdType.java:
##
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Represents the sequence ID types managed by {@link SequenceIdGenerator} and
their persisted RocksDB keys.
+ */
+public enum SequenceIdType {
+
+ LOCAL_ID("localId"),
+
+ DEL_TXN_ID("delTxnId"),
+
+ CONTAINER_ID("containerId"),
+
+ /** Certificate ID for all services, including root certificates. */
+ CERTIFICATE_ID("CertificateId"),
+
+ /**
+ * @deprecated Use {@link #CERTIFICATE_ID} instead.
+ */
+ @Deprecated
+ ROOT_CERTIFICATE_ID("rootCertificateId");
+
+ /**
+ * The key string stored in the RocksDB sequenceId table.
+ */
+ private final String dbKey;
+
+ /**
+ * Reverse lookup map from db key string to enum constant.
+ */
+ private static final Map DB_KEY_MAP;
+
+ static {
+Map map = new HashMap<>();
+for (SequenceIdType type : values()) {
+ map.put(type.dbKey, type);
+}
+DB_KEY_MAP = Collections.unmodifiableMap(map);
+ }
+
+ SequenceIdType(String dbKey) {
+this.dbKey = dbKey;
+ }
+
+ /**
+ * Returns the key string used to persist this sequence ID in RocksDB.
+ * This value must not be changed to keep backward compatibility with
+ * existing databases.
+ */
+ public String getDbKey() {
+return dbKey;
+ }
+
+ /**
+ * Returns the {@link SequenceIdType} corresponding to the provided RocksDB
key string, or null if unmapped.
+ */
+ public static SequenceIdType fromDbKey(String dbKey) {
Review Comment:
Done.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
navinko commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3219308288
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -64,19 +64,19 @@ public class SequenceIdGenerator {
/**
* Ids supported.
*/
- public static final String LOCAL_ID = "localId";
- public static final String DEL_TXN_ID = "delTxnId";
- public static final String CONTAINER_ID = "containerId";
+ public static final String LOCAL_ID = SequenceIdType.LOCAL_ID.getDbKey();
+ public static final String DEL_TXN_ID = SequenceIdType.DEL_TXN_ID.getDbKey();
+ public static final String CONTAINER_ID =
SequenceIdType.CONTAINER_ID.getDbKey();
// Certificate ID for all services, including root certificates, whose ID
// were using "rootCertificateId" before.
- public static final String CERTIFICATE_ID = "CertificateId";
+ public static final String CERTIFICATE_ID =
SequenceIdType.CERTIFICATE_ID.getDbKey();
@Deprecated
- public static final String ROOT_CERTIFICATE_ID = "rootCertificateId";
+ public static final String ROOT_CERTIFICATE_ID =
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey();
Review Comment:
Done
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -89,7 +89,7 @@ public class SequenceIdGenerator {
*/
public SequenceIdGenerator(ConfigurationSource conf,
SCMHAManager scmhaManager, Table sequenceIdTable) {
-this.sequenceIdToBatchMap = new HashMap<>();
+this.sequenceIdToBatchMap = new EnumMap<>(SequenceIdType.class);
Review Comment:
Done
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3210641284
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -128,18 +128,18 @@ public long getNextId(String sequenceIdName) throws
SCMException {
Preconditions.checkArgument(Long.MAX_VALUE - batch.lastId >=
batchSize);
long nextLastId = batch.lastId +
-((sequenceIdName.equals(CERTIFICATE_ID)) ? 1 : batchSize);
+(idType == SequenceIdType.CERTIFICATE_ID ? 1 : batchSize);
-if (stateManager.allocateBatch(sequenceIdName,
+if (stateManager.allocateBatch(idType.getDbKey(),
prevLastId, nextLastId)) {
batch.lastId = nextLastId;
LOG.info("Allocate a batch for {}, change lastId from {} to {}.",
- sequenceIdName, prevLastId, batch.lastId);
+ idType, prevLastId, batch.lastId);
break;
}
// reload lastId from RocksDB.
-batch.lastId = stateManager.getLastId(sequenceIdName);
+batch.lastId = stateManager.getLastId(idType.getDbKey());
Review Comment:
- Change the parameter to SequenceIdType
- Change StateManagerImpl.sequenceIdToLastIdMap key to SequenceIdType
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
szetszwo commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3210589616
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdType.java:
##
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Represents the sequence ID types managed by {@link SequenceIdGenerator} and
their persisted RocksDB keys.
+ */
+public enum SequenceIdType {
+
+ LOCAL_ID("localId"),
+
+ DEL_TXN_ID("delTxnId"),
+
+ CONTAINER_ID("containerId"),
+
+ /** Certificate ID for all services, including root certificates. */
+ CERTIFICATE_ID("CertificateId"),
+
+ /**
+ * @deprecated Use {@link #CERTIFICATE_ID} instead.
+ */
+ @Deprecated
+ ROOT_CERTIFICATE_ID("rootCertificateId");
+
+ /**
+ * The key string stored in the RocksDB sequenceId table.
+ */
+ private final String dbKey;
+
+ /**
+ * Reverse lookup map from db key string to enum constant.
+ */
+ private static final Map DB_KEY_MAP;
+
+ static {
+Map map = new HashMap<>();
+for (SequenceIdType type : values()) {
+ map.put(type.dbKey, type);
+}
+DB_KEY_MAP = Collections.unmodifiableMap(map);
+ }
+
+ SequenceIdType(String dbKey) {
+this.dbKey = dbKey;
+ }
+
+ /**
+ * Returns the key string used to persist this sequence ID in RocksDB.
+ * This value must not be changed to keep backward compatibility with
+ * existing databases.
+ */
+ public String getDbKey() {
+return dbKey;
+ }
+
+ /**
+ * Returns the {@link SequenceIdType} corresponding to the provided RocksDB
key string, or null if unmapped.
+ */
+ public static SequenceIdType fromDbKey(String dbKey) {
Review Comment:
This method is unused except testReturnsNullIfEnumConstantNotAvailable().
Let's remove it and also DB_KEY_MAP.
Indeed, we should always use SequenceIdType and never convert a String to
SequenceIdType.
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -64,19 +64,19 @@ public class SequenceIdGenerator {
/**
* Ids supported.
*/
- public static final String LOCAL_ID = "localId";
- public static final String DEL_TXN_ID = "delTxnId";
- public static final String CONTAINER_ID = "containerId";
+ public static final String LOCAL_ID = SequenceIdType.LOCAL_ID.getDbKey();
+ public static final String DEL_TXN_ID = SequenceIdType.DEL_TXN_ID.getDbKey();
+ public static final String CONTAINER_ID =
SequenceIdType.CONTAINER_ID.getDbKey();
// Certificate ID for all services, including root certificates, whose ID
// were using "rootCertificateId" before.
- public static final String CERTIFICATE_ID = "CertificateId";
+ public static final String CERTIFICATE_ID =
SequenceIdType.CERTIFICATE_ID.getDbKey();
@Deprecated
- public static final String ROOT_CERTIFICATE_ID = "rootCertificateId";
+ public static final String ROOT_CERTIFICATE_ID =
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey();
Review Comment:
Change them to private.
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -89,7 +89,7 @@ public class SequenceIdGenerator {
*/
public SequenceIdGenerator(ConfigurationSource conf,
SCMHAManager scmhaManager, Table sequenceIdTable) {
-this.sequenceIdToBatchMap = new HashMap<>();
+this.sequenceIdToBatchMap = new EnumMap<>(SequenceIdType.class);
Review Comment:
Make it unmodifiable:
```java
this.sequenceIdToBatchMap = newSequenceIdToBatchMap();
```
```java
static Map newSequenceIdToBatchMap() {
final EnumMap map = new
EnumMap<>(SequenceIdType.class);
for (SequenceIdType type : SequenceIdType.values()) {
map.put(type, new Batch());
}
return Collections.unmodifiableMap(map);
}
```
##
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java:
##
@@ -108,14 +108,14 @@ public StateManager createStateManager(SCMHAManager
scmhaManager,
}
/**
- * @p
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
sreejasahithi commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3207107915
##
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIdType.java:
##
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link SequenceIdType}.
+ */
+public class TestSequenceIdType {
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testStringSyncWithEnumConstants() {
+// Even though we changed to an Enum, these underlying strings are
+// persisted in RocksDB. Do not change them.
+assertEquals("localId", SequenceIdType.LOCAL_ID.getDbKey());
+assertEquals("delTxnId", SequenceIdType.DEL_TXN_ID.getDbKey());
+assertEquals("containerId", SequenceIdType.CONTAINER_ID.getDbKey());
+assertEquals("CertificateId", SequenceIdType.CERTIFICATE_ID.getDbKey());
+assertEquals("rootCertificateId",
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey());
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedStringSyncWithEnumConstants() {
+assertEquals(SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey(),
+SequenceIdGenerator.ROOT_CERTIFICATE_ID);
+ }
+
+ @Test
+ public void testNumberOfEnumConstants() {
+// If a new SequenceIdType is added, this test will fail.
+// This serves as a reminder to the developer to verify RocksDB backward
+// compatibility and update this test class accordingly.
+assertEquals(5, SequenceIdType.values().length);
+ }
+
+ @Test
+ public void testIfNewEnumConstantGetsAdded() {
+Set expectedNames = new HashSet<>(Arrays.asList(
+"LOCAL_ID", "DEL_TXN_ID", "CONTAINER_ID",
+"CERTIFICATE_ID", "ROOT_CERTIFICATE_ID"));
+
+Set actualNames = new HashSet<>();
+for (SequenceIdType type : SequenceIdType.values()) {
+ actualNames.add(type.name());
+}
+
+// Filter exactly what changed to make the failure message extremely clear
+Set added = new HashSet<>(actualNames);
+added.removeAll(expectedNames);
+
+Set removed = new HashSet<>(expectedNames);
+removed.removeAll(actualNames);
+
+assertTrue(added.isEmpty() && removed.isEmpty(),
+() -> "SequenceIdType constants changed!\n" +
+"Unexpectedly Added: " + added + "\n" +
+"Unexpectedly Removed: " + removed + "\n" +
+"ACTION REQUIRED: If this change is intentional, you MUST verify "
+
+"RocksDB backward compatibility and update this test's
expectedNames.");
+ }
+
+ @Test
+ public void testReturnsNullIfEnumConstantNotAvailable() {
+assertNull(SequenceIdType.fromDbKey("unmapped-key-string"));
+assertNull(SequenceIdType.fromDbKey(null));
+ }
Review Comment:
We can in a different test consider asserting assertSame(type,
fromDbKey(type.getDbKey())) for every constant so the static map stays aligned
with getDbKey(), not covered by the unknown/null case alone.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
sreejasahithi commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3207107915
##
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIdType.java:
##
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link SequenceIdType}.
+ */
+public class TestSequenceIdType {
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testStringSyncWithEnumConstants() {
+// Even though we changed to an Enum, these underlying strings are
+// persisted in RocksDB. Do not change them.
+assertEquals("localId", SequenceIdType.LOCAL_ID.getDbKey());
+assertEquals("delTxnId", SequenceIdType.DEL_TXN_ID.getDbKey());
+assertEquals("containerId", SequenceIdType.CONTAINER_ID.getDbKey());
+assertEquals("CertificateId", SequenceIdType.CERTIFICATE_ID.getDbKey());
+assertEquals("rootCertificateId",
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey());
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedStringSyncWithEnumConstants() {
+assertEquals(SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey(),
+SequenceIdGenerator.ROOT_CERTIFICATE_ID);
+ }
+
+ @Test
+ public void testNumberOfEnumConstants() {
+// If a new SequenceIdType is added, this test will fail.
+// This serves as a reminder to the developer to verify RocksDB backward
+// compatibility and update this test class accordingly.
+assertEquals(5, SequenceIdType.values().length);
+ }
+
+ @Test
+ public void testIfNewEnumConstantGetsAdded() {
+Set expectedNames = new HashSet<>(Arrays.asList(
+"LOCAL_ID", "DEL_TXN_ID", "CONTAINER_ID",
+"CERTIFICATE_ID", "ROOT_CERTIFICATE_ID"));
+
+Set actualNames = new HashSet<>();
+for (SequenceIdType type : SequenceIdType.values()) {
+ actualNames.add(type.name());
+}
+
+// Filter exactly what changed to make the failure message extremely clear
+Set added = new HashSet<>(actualNames);
+added.removeAll(expectedNames);
+
+Set removed = new HashSet<>(expectedNames);
+removed.removeAll(actualNames);
+
+assertTrue(added.isEmpty() && removed.isEmpty(),
+() -> "SequenceIdType constants changed!\n" +
+"Unexpectedly Added: " + added + "\n" +
+"Unexpectedly Removed: " + removed + "\n" +
+"ACTION REQUIRED: If this change is intentional, you MUST verify "
+
+"RocksDB backward compatibility and update this test's
expectedNames.");
+ }
+
+ @Test
+ public void testReturnsNullIfEnumConstantNotAvailable() {
+assertNull(SequenceIdType.fromDbKey("unmapped-key-string"));
+assertNull(SequenceIdType.fromDbKey(null));
+ }
Review Comment:
We can in a different test consider asserting assertSame(type,
fromDbKey(type.getDbKey())) for every constant so the static map stays aligned
with getDbKey(), not covered by the unmapped/null case alone.
--
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]
Re: [PR] HDDS-15145. Use enum for the ID type in SequenceIdGenerator [ozone]
sreejasahithi commented on code in PR #10211:
URL: https://github.com/apache/ozone/pull/10211#discussion_r3206787775
##
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIdType.java:
##
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hdds.scm.ha;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link SequenceIdType}.
+ */
+public class TestSequenceIdType {
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testStringSyncWithEnumConstants() {
+// Even though we changed to an Enum, these underlying strings are
+// persisted in RocksDB. Do not change them.
+assertEquals("localId", SequenceIdType.LOCAL_ID.getDbKey());
+assertEquals("delTxnId", SequenceIdType.DEL_TXN_ID.getDbKey());
+assertEquals("containerId", SequenceIdType.CONTAINER_ID.getDbKey());
+assertEquals("CertificateId", SequenceIdType.CERTIFICATE_ID.getDbKey());
+assertEquals("rootCertificateId",
SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey());
+ }
+
+ @Test
+ @SuppressWarnings("deprecation")
+ public void testDeprecatedStringSyncWithEnumConstants() {
+assertEquals(SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey(),
+SequenceIdGenerator.ROOT_CERTIFICATE_ID);
+ }
+
+ @Test
+ public void testNumberOfEnumConstants() {
+// If a new SequenceIdType is added, this test will fail.
+// This serves as a reminder to the developer to verify RocksDB backward
+// compatibility and update this test class accordingly.
+assertEquals(5, SequenceIdType.values().length);
+ }
Review Comment:
Here testNumberOfEnumConstants (assertEquals(5,
SequenceIdType.values().length)) is fully subsumed by
testIfNewEnumConstantGetsAdded. Every failure scenario that
testNumberOfEnumConstants catches is also caught by
testIfNewEnumConstantGetsAdded, which additionally catches renames (count stays
5 but a name changes), and provides a far more descriptive failure message with
the "ACTION REQUIRED" instruction.
--
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]
