You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/11/08 10:33:10 UTC
[GitHub] [iotdb] Cpaulyz opened a new pull request, #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.
Cpaulyz opened a new pull request, #7936:
URL: https://github.com/apache/iotdb/pull/7936
## Description
Currently, the SchemaRegionSchemaFileImpl relies on mlog and schema file to recover.
In cluster mode, schemaRegion relies on Ratis to guarantee consistency, while raftlog takes repeating info of mlog.
We want to use raftlog to recover info not in snapshot and use schema file to implement snapshot and online business.
This PR includes the implmentation of these two interface in `SchemaRegionSchemaFileImpl`:
1. createSnapshot
2. loadSnapshot
--
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
[GitHub] [iotdb] Cpaulyz commented on a diff in pull request #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.
Posted by GitBox <gi...@apache.org>.
Cpaulyz commented on code in PR #7936:
URL: https://github.com/apache/iotdb/pull/7936#discussion_r1017581823
##########
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 an 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
[GitHub] [iotdb] Cpaulyz commented on a diff in pull request #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.
Posted by 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
[GitHub] [iotdb] MarcosZyk commented on a diff in pull request #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.
Posted by GitBox <gi...@apache.org>.
MarcosZyk commented on code in PR #7936:
URL: https://github.com/apache/iotdb/pull/7936#discussion_r1019015868
##########
server/src/main/java/org/apache/iotdb/db/metadata/schemaregion/SchemaRegionSchemaFileImpl.java:
##########
@@ -164,13 +165,16 @@ public class SchemaRegionSchemaFileImpl implements ISchemaRegion {
private volatile boolean initialized = false;
private boolean isClearing = false;
- private String schemaRegionDirPath;
- private String storageGroupFullPath;
- private SchemaRegionId schemaRegionId;
+ private final String storageGroupDirPath;
+ private final String schemaRegionDirPath;
+ private final String storageGroupFullPath;
+ private final SchemaRegionId schemaRegionId;
+ // the log file writer
+ private boolean usingMLog = true;
// the log file seriesPath
- private String logFilePath;
- private File logFile;
+ // private String logFilePath;
+ // private File logFile;
Review Comment:
remove this
--
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
[GitHub] [iotdb] bigreybear commented on a diff in pull request #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.
Posted by GitBox <gi...@apache.org>.
bigreybear commented on code in PR #7936:
URL: https://github.com/apache/iotdb/pull/7936#discussion_r1017511654
##########
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 ?
##########
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?
--
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
[GitHub] [iotdb] MarcosZyk merged pull request #7936: [IOTDB-4838] Adapt SchemaRegionSchemaFileImpl's recovery to Ratis.
Posted by GitBox <gi...@apache.org>.
MarcosZyk merged PR #7936:
URL: https://github.com/apache/iotdb/pull/7936
--
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