[GitHub] [iotdb] Cpaulyz commented on a diff in pull request #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.

[GitBox <gi...@apache.org>]

Cpaulyz commented on code in PR #7936:
URL: https://github.com/apache/iotdb/pull/7936#discussion_r1017581781


##########
server/src/main/java/org/apache/iotdb/db/metadata/mtree/store/disk/schemafile/SchemaFile.java:
##########
@@ -405,4 +412,48 @@ public long getTargetSegmentOnTest(long srcSegAddr, String key)
 
   // endregion
 
+  // region Snapshot
+
+  @Override
+  public boolean createSnapshot(File snapshotDir) {
+    File schemaFileSnapshot =
+        SystemFileFactory.INSTANCE.getFile(snapshotDir, MetadataConstant.SCHEMA_FILE_SNAPSHOT);
+    try {
+      sync();
+      if (schemaFileSnapshot.exists() && !schemaFileSnapshot.delete()) {
+        logger.error(
+            "Failed to delete old snapshot {} while creating schema file snapshot.",
+            schemaFileSnapshot.getName());
+        return false;
+      }
+      Files.copy(Paths.get(filePath), schemaFileSnapshot.toPath());
+      return true;
+    } catch (IOException e) {
+      logger.error("Failed to create SchemaFile snapshot due to {}", e.getMessage(), e);
+      schemaFileSnapshot.delete();
+      return false;
+    }
+  }
+
+  public static ISchemaFile loadSnapshot(File snapshotDir, String sgName, int schemaRegionId)
+      throws IOException, MetadataException {
+    File snapshot =
+        SystemFileFactory.INSTANCE.getFile(snapshotDir, MetadataConstant.SCHEMA_FILE_SNAPSHOT);
+    if (!snapshot.exists()) {
+      throw new SchemaFileNotExists(snapshot.getPath());
+    }
+    File schemaFile =
+        SystemFileFactory.INSTANCE.getFile(
+            getDirPath(sgName, schemaRegionId), MetadataConstant.SCHEMA_FILE_NAME);
+    File schemaLogFile =
+        SystemFileFactory.INSTANCE.getFile(
+            getDirPath(sgName, schemaRegionId), MetadataConstant.SCHEMA_LOG_FILE_NAME);
+    Files.deleteIfExists(schemaFile.toPath());
+    Files.deleteIfExists(schemaLogFile.toPath());
+    Files.createLink(schemaFile.toPath(), snapshot.toPath());
+    return new SchemaFile(sgName, schemaRegionId, false, -1L, false);

Review Comment:
   > I see a `-1L` for parameter of TTL, I'm not sure if it is ok for snapshot if the original SchemaFile had been set a ttl not equals -1L ?
   
   Change to `CommonDescriptor.getInstance().getConfig().getDefaultTTLInMs()`. But it seems that whatever this field is, it will be overwritten in the `initFileHeader()`.



##########
server/src/test/java/org/apache/iotdb/db/metadata/schemaRegion/SchemaRegionBasicTest.java:
##########
@@ -86,6 +95,103 @@ public void tearDown() throws Exception {
     config.setClusterMode(isClusterMode);
   }
 
+  @Test
+  public void testRatisModeSnapshot() throws Exception {
+    String schemaRegionConsensusProtocolClass = config.getSchemaRegionConsensusProtocolClass();
+    config.setSchemaRegionConsensusProtocolClass(ConsensusFactory.RATIS_CONSENSUS);
+    try {
+      PartialPath storageGroup = new PartialPath("root.sg");
+      SchemaRegionId schemaRegionId = new SchemaRegionId(0);
+      SchemaEngine.getInstance().createSchemaRegion(storageGroup, schemaRegionId);
+      ISchemaRegion schemaRegion = SchemaEngine.getInstance().getSchemaRegion(schemaRegionId);
+
+      File mLogFile =
+          SystemFileFactory.INSTANCE.getFile(
+              schemaRegion.getStorageGroupFullPath()
+                  + File.separator
+                  + schemaRegion.getSchemaRegionId().getId(),
+              MetadataConstant.METADATA_LOG);
+      Assert.assertFalse(mLogFile.exists());
+
+      Map<String, String> tags = new HashMap<>();
+      tags.put("tag-key", "tag-value");
+      schemaRegion.createTimeseries(
+          new CreateTimeSeriesPlan(
+              new PartialPath("root.sg.d1.s1"),
+              TSDataType.INT32,
+              TSEncoding.PLAIN,
+              CompressionType.UNCOMPRESSED,
+              null,
+              tags,
+              null,
+              null),
+          -1);
+
+      File snapshotDir = new File(config.getSchemaDir() + File.separator + "snapshot");
+      snapshotDir.mkdir();
+      schemaRegion.createSnapshot(snapshotDir);
+
+      schemaRegion.loadSnapshot(snapshotDir);

Review Comment:
   > I am not sure if it is better with another object `SchemaRegion` to load the snapshot and verify the test?
   
   I added a addional test case to use another object `SchemaRegion`.
   



-- 
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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org