hemantk-12 commented on code in PR #5223:
URL: https://github.com/apache/ozone/pull/5223#discussion_r1308058060
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotChain.java:
##########
@@ -273,4 +290,68 @@ public void testChainFromLoadFromTable() throws Exception {
() -> chainManager.previousPathSnapshot(String
.join("/", "vol1", "bucket1"), snapshotID1));
}
+
+ private static Stream<? extends Arguments> invalidSnapshotChain() {
+ List<UUID> nodes = IntStream.range(0, 5)
+ .mapToObj(i -> UUID.randomUUID())
+ .collect(Collectors.toList());
+ return Stream.of(
+ Arguments.of(nodes, Named.of("Disconnected Snapshot Chain",
+ ImmutableMap.of(
+ nodes.get(1), nodes.get(0),
+ nodes.get(2), nodes.get(1),
+ nodes.get(4), nodes.get(3)))),
+ Arguments.of(nodes, Named.of("Complete Cyclic Snapshot Chain",
+ ImmutableMap.of(
+ nodes.get(0), nodes.get(4),
+ nodes.get(1), nodes.get(0),
+ nodes.get(2), nodes.get(1),
+ nodes.get(3), nodes.get(2),
+ nodes.get(4), nodes.get(3)))),
+ Arguments.of(nodes, Named.of("Partial Cyclic Snapshot Chain",
+ ImmutableMap.of(
+ nodes.get(0), nodes.get(3),
+ nodes.get(1), nodes.get(0),
+ nodes.get(2), nodes.get(1),
+ nodes.get(3), nodes.get(2),
+ nodes.get(4), nodes.get(3)))),
+ Arguments.of(nodes, Named.of("Diverged Snapshot Chain",
+ ImmutableMap.of(nodes.get(1), nodes.get(0),
+ nodes.get(2), nodes.get(1),
+ nodes.get(3), nodes.get(2),
+ nodes.get(4), nodes.get(2))))
+ );
+ }
+
+ @ParameterizedTest
+ @MethodSource("invalidSnapshotChain")
+ public void testInvalidGlobalChainFromLoadFromTable(
+ List<UUID> snapshotIDs, Map<UUID, UUID> snapshotChain) throws Exception {
+ Table<String, SnapshotInfo> snapshotInfo =
+ omMetadataManager.getSnapshotInfoTable();
+ for (UUID snapshotID : snapshotIDs) {
+ snapshotInfo.put(snapshotID.toString(),
+ createSnapshotInfo(snapshotID, snapshotChain.get(snapshotID),
+ snapshotChain.get(snapshotID), System.currentTimeMillis()));
+ }
+ Assertions.assertThrows(IllegalStateException.class,
+ () -> new SnapshotChainManager(omMetadataManager));
+ }
+
+ @ParameterizedTest
+ @MethodSource("invalidSnapshotChain")
+ public void testInvalidChainFromLoadFromTable(List<UUID> snapshotIDs,
+ Map<UUID, UUID> snapshotChain) throws Exception {
+ Table<String, SnapshotInfo> snapshotInfo =
+ omMetadataManager.getSnapshotInfoTable();
+ UUID prevSnapshotId = null;
+ for (UUID snapshotID : snapshotIDs) {
+ snapshotInfo.put(snapshotID.toString(),
+ createSnapshotInfo(snapshotID, snapshotChain.get(snapshotID),
+ prevSnapshotId, System.currentTimeMillis()));
+ prevSnapshotId = snapshotID;
+ }
+ Assertions.assertThrows(RuntimeException.class,
Review Comment:
Why this is not `IllegalStateException`? I think you can change it to
`IllegalStateException` after changing all the exception from `IOException` to
`IllegalStateException`.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -71,7 +71,27 @@ public SnapshotChainManager(OMMetadataManager
metadataManager) {
private void addSnapshotGlobal(UUID snapshotID,
UUID prevGlobalID) throws IOException {
// On add snapshot, set previous snapshot entry nextSnapshotID = snapshotID
+ if (globalSnapshotChain.containsKey(snapshotID)) {
+ throw new IllegalStateException(String.format(
+ "Snapshot chain corruption. Snapshot with snapshotId: %s is " +
+ "already present in the the chain.", snapshotID));
+ }
+ if (globalSnapshotChain.size() > 0 && prevGlobalID == null) {
+ throw new IllegalStateException(String.format("Snapshot chain " +
+ "corruption. Adding snapshot %s as head node while there are %d in "
+
+ "the global snapshot chain.", snapshotID,
+ globalSnapshotChain.size()));
+ }
if (prevGlobalID != null && globalSnapshotChain.containsKey(prevGlobalID))
{
Review Comment:
nit: Can we move this `if` block after line 104? Basically move `if` block
at line 98-104 before this `if `block
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -71,7 +71,27 @@ public SnapshotChainManager(OMMetadataManager
metadataManager) {
private void addSnapshotGlobal(UUID snapshotID,
UUID prevGlobalID) throws IOException {
// On add snapshot, set previous snapshot entry nextSnapshotID = snapshotID
+ if (globalSnapshotChain.containsKey(snapshotID)) {
+ throw new IllegalStateException(String.format(
+ "Snapshot chain corruption. Snapshot with snapshotId: %s is " +
+ "already present in the the chain.", snapshotID));
Review Comment:
```suggestion
"already present in the chain.", snapshotID));
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -105,7 +125,26 @@ private void addSnapshotPath(String snapshotPath,
snapshotID));
}
+ if (prevPathID == null && snapshotChainByPath.containsKey(snapshotPath) &&
+ !snapshotChainByPath.get(snapshotPath).isEmpty()) {
+ throw new IllegalStateException(String.format(
+ "Snapshot chain corruption. Error while adding snapshot with " +
+ "snapshotId %s with as the first snapshot in snapshot path:" +
+ " %s which already %d snapshots.", snapshotID, snapshotPath,
+ snapshotChainByPath.get(snapshotPath).size()));
+ }
+
if (prevPathID != null && snapshotChainByPath.containsKey(snapshotPath)) {
+ if (snapshotChainByPath.get(snapshotPath).get(prevPathID)
+ .getNextSnapshotId() != null) {
+ throw new IOException(String.format("Snapshot chain corruption. " +
Review Comment:
Please use `IllegalStateException`. Also change it for line number 100 and
122 and remove `IOException` from the method signature as well.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -71,7 +71,27 @@ public SnapshotChainManager(OMMetadataManager
metadataManager) {
private void addSnapshotGlobal(UUID snapshotID,
UUID prevGlobalID) throws IOException {
// On add snapshot, set previous snapshot entry nextSnapshotID = snapshotID
+ if (globalSnapshotChain.containsKey(snapshotID)) {
+ throw new IllegalStateException(String.format(
+ "Snapshot chain corruption. Snapshot with snapshotId: %s is " +
+ "already present in the the chain.", snapshotID));
+ }
+ if (globalSnapshotChain.size() > 0 && prevGlobalID == null) {
+ throw new IllegalStateException(String.format("Snapshot chain " +
+ "corruption. Adding snapshot %s as head node while there are %d in "
+
+ "the global snapshot chain.", snapshotID,
+ globalSnapshotChain.size()));
+ }
if (prevGlobalID != null && globalSnapshotChain.containsKey(prevGlobalID))
{
+ if (globalSnapshotChain.get(prevGlobalID).getNextSnapshotId() != null) {
Review Comment:
nit: Should we add `hasNextSnapshotId` and `hasPreviousSnapshotId` in
`SnapshotChainInfo`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -234,7 +273,10 @@ private void loadFromSnapshotInfoTable(OMMetadataManager
metadataManager) {
// snapshot chains - both global and local path
try (TableIterator<String, ? extends Table.KeyValue<String, SnapshotInfo>>
keyIter = metadataManager.getSnapshotInfoTable().iterator()) {
- Map<Long, SnapshotInfo> snaps = new TreeMap<>();
+ Map<UUID, SnapshotInfo> snaps = new HashMap<>();
+ // Forward Linked list for snapshot chain.
+ Map<UUID, UUID> nextSnapshotIdMap = new HashMap<>();
Review Comment:
nit: may be `snapshotToNextSnapshotMap`
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotChain.java:
##########
@@ -239,8 +253,10 @@ public void testDeleteSnapshot() throws Exception {
assertEquals(snapshotID3,
chainManager.previousGlobalSnapshot(snapshotID2));
}
- @Test
- public void testChainFromLoadFromTable() throws Exception {
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testChainFromLoadFromTable(boolean increasingTIme)
Review Comment:
I think it is unnecessary to make it parameterized test. Just use random
number generation
[here](https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotChain.java#L75)
because snapshot chain is independent of snapshot creation. It could be
increasing, decreasing or mix of both.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -71,7 +71,27 @@ public SnapshotChainManager(OMMetadataManager
metadataManager) {
private void addSnapshotGlobal(UUID snapshotID,
UUID prevGlobalID) throws IOException {
// On add snapshot, set previous snapshot entry nextSnapshotID = snapshotID
+ if (globalSnapshotChain.containsKey(snapshotID)) {
+ throw new IllegalStateException(String.format(
+ "Snapshot chain corruption. Snapshot with snapshotId: %s is " +
Review Comment:
nit: Can you please change exception message to `Global snapshot chain
corruption` for all the exception messages in `addSnapshotGlobal` and `Path
level snapshot chain corruption` for the exception messages in
`addSnapshotPath`? And assert that exception message starts with correct
message in the test.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -105,7 +125,26 @@ private void addSnapshotPath(String snapshotPath,
snapshotID));
}
+ if (prevPathID == null && snapshotChainByPath.containsKey(snapshotPath) &&
+ !snapshotChainByPath.get(snapshotPath).isEmpty()) {
+ throw new IllegalStateException(String.format(
+ "Snapshot chain corruption. Error while adding snapshot with " +
Review Comment:
```suggestion
"Snapshot chain corruption. Error while adding snapshot with " +
"snapshotId: %s as the first snapshot in snapshot path: %s " +
"which already has %d snapshots.", snapshotID, snapshotPath,
```
--
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]
