hemantk-12 commented on code in PR #5223:
URL: https://github.com/apache/ozone/pull/5223#discussion_r1305970863
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -234,7 +234,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> snapshotChain = new HashMap<>();
Review Comment:
nit: I think variable name and comment are not correct. To me, it is not a
chain. It is a map of snapshot to next snapshot. Chain is something order is
maintain.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotChain.java:
##########
@@ -273,4 +286,56 @@ 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(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(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(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(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 testInvalidChainFromLoadFromTable(Map<UUID, UUID> snapshotChain)
Review Comment:
Should we add similar test for path level previous?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java:
##########
@@ -243,10 +246,37 @@ private void loadFromSnapshotInfoTable(OMMetadataManager
metadataManager) {
while (keyIter.hasNext()) {
kv = keyIter.next();
- snaps.put(kv.getValue().getCreationTime(), kv.getValue());
+ SnapshotInfo snapshotInfo = kv.getValue();
+ snaps.put(kv.getValue().getSnapshotId(), snapshotInfo);
+ if (snapshotInfo.getGlobalPreviousSnapshotId() != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Global Snapshot chain link {} -> {}",
+ snapshotInfo.getGlobalPreviousSnapshotId(),
+ snapshotInfo.getSnapshotId());
+ }
+ // Adding edge to the linked list. prevGlobalSnapId -> snapId
+ snapshotChain.put(snapshotInfo.getGlobalPreviousSnapshotId(),
+ snapshotInfo.getSnapshotId());
+ } else {
+ head = snapshotInfo.getSnapshotId();
+ }
+ }
+ int size = 0;
+ UUID prev = null;
+ while (head != null) {
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Adding Snapshot Info: {}", snaps.get(head));
+ }
+ addSnapshot(snaps.get(head));
+ size += 1;
+ prev = head;
+ head = snapshotChain.get(head);
}
- for (SnapshotInfo snapshotInfo : snaps.values()) {
- addSnapshot(snapshotInfo);
+ if (size != snaps.size()) {
+ throw new IOException(String.format("Snapshot chain corruption. " +
Review Comment:
nit:
1. Please change it to `IllegalStateException`.
2. Exception message could be better. May be `(Snapshot chain corruption.
All snapshots have not been added to the snapshot chain. Last snapshot added to
chain : %s", prev)`.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestSnapshotChain.java:
##########
@@ -65,14 +76,15 @@ public void setup() throws Exception {
private SnapshotInfo createSnapshotInfo(UUID snapshotID,
UUID pathPrevID,
- UUID globalPrevID) {
+ UUID globalPrevID,
+ long creationTime) {
return new SnapshotInfo.Builder()
.setSnapshotId(snapshotID)
.setName("test")
.setVolumeName("vol1")
.setBucketName("bucket1")
.setSnapshotStatus(SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)
- .setCreationTime(Time.now())
+ .setCreationTime(creationTime)
Review Comment:
qq: Do we really need pass creation time? Snapshot chain construction is
independent of it? Isn't 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]
