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/05/23 07:57:54 UTC
[GitHub] [iotdb] SzyWilliam opened a new pull request, #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
SzyWilliam opened a new pull request, #5991:
URL: https://github.com/apache/iotdb/pull/5991
Recently Apache Ratis Community releases 2.3.0, which includes bugfix to known InstallSnapshot issues.
In this PR, I upgrade Ratis version to 2.3.0.
Notice:
Currently, we name the snapshotDir with Term_Index so that we can tell which directory contains
the latest snapshot files. Unfortunately, when leader install snapshot to a slow follower,
current Ratis implementation will flatten the directory and place all the snapshots directly
under statemachine dir. Under this scenario, we cannot restore Term_Index from directory name.
We decided to add an empty metadata file containing only Term_Index into the snapshotDir. his
metadata file will be installed along with application snapshot files, so that Term_Index
information is kept during InstallSnapshot.
--
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] OneSizeFitsQuorum commented on a diff in pull request #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #5991:
URL: https://github.com/apache/iotdb/pull/5991#discussion_r880524307
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/ApplicationStateMachineProxy.java:
##########
@@ -142,13 +144,22 @@ public long takeSnapshot() throws IOException {
// require the application statemachine to take the latest snapshot
String metadata = Utils.getMetadataFromTermIndex(lastApplied);
File snapshotDir = snapshotStorage.getSnapshotDir(metadata);
+
+ // delete snapshotDir fully in case of last failed take snapshot
Review Comment:
typo
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/ApplicationStateMachineProxy.java:
##########
@@ -142,13 +144,22 @@ public long takeSnapshot() throws IOException {
// require the application statemachine to take the latest snapshot
String metadata = Utils.getMetadataFromTermIndex(lastApplied);
File snapshotDir = snapshotStorage.getSnapshotDir(metadata);
+
+ // delete snapshotDir fully in case of last failed take snapshot
+ FileUtils.deleteFully(snapshotDir);
+
snapshotDir.mkdir();
if (!snapshotDir.isDirectory()) {
logger.error("Unable to create snapshotDir at {}", snapshotDir);
return RaftLog.INVALID_LOG_INDEX;
}
- boolean success = applicationStateMachine.takeSnapshot(snapshotDir);
- if (!success) {
+
+ boolean applicationTakeSnapshotSuccess = applicationStateMachine.takeSnapshot(snapshotDir);
+ boolean addTermIndexMetafileSuccess =
+ snapshotStorage.addTermIndexMetaFile(snapshotDir, metadata);
Review Comment:
Do we need to clean up this if failed?
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/SnapshotStorage.java:
##########
@@ -141,4 +137,75 @@ public File getStateMachineDir() {
public File getSnapshotDir(String snapshotMetadata) {
return new File(stateMachineDir.getAbsolutePath() + File.separator + snapshotMetadata);
}
+
+ /**
+ * Currently, we name the snapshotDir with Term_Index so that we can tell which directory contains
+ * the latest snapshot files. Unfortunately, when leader install snapshot to a slow follower,
+ * current Ratis implementation will flatten the directory and place all the snapshots directly
+ * under statemachine dir. Under this scenario, we cannot restore Term_Index from directory name.
+ * We decided to add an empty metadata file containing only Term_Index into the snapshotDir. his
+ * metadata file will be installed along with application snapshot files, so that Term_Index
+ * information is kept during InstallSnapshot.
+ */
+ public boolean addTermIndexMetaFile(File snapshotDir, String termIndexMetadata) {
+ File snapshotMetaFile = new File(getMetafilePath(snapshotDir, termIndexMetadata));
+ try {
+ return snapshotMetaFile.createNewFile();
+ } catch (IOException e) {
+ logger.warn("cannot create snapshot metafile: ", e);
+ return false;
+ }
+ }
+
+ private String getMetafilePath(File snapshotDir, String termIndexMetadata) {
+ // e.g. /_sm/3_39/.ratis_meta.3_39
+ return snapshotDir.getAbsolutePath() + File.separator + META_FILE_PREFIX + termIndexMetadata;
+ }
+
+ private String getMetafileMatcherRegex() {
+ // meta file should always end with term_index
+ return META_FILE_PREFIX + "\\d+_\\d+$";
+ }
+
+ /**
+ * After leader InstallSnapshot to a slow follower, Ratis will put all snapshot files directly
+ * under statemachineDir. We need to handle this special scenario and rearrange these files to
+ * appropriate sub-directory this function will move all snapshot files directly under /sm to
+ * /sm/term_index/
+ */
+ void rearrangeSMDirHierarchyIfNecessary() {
Review Comment:
The function naming is invalid
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/SnapshotStorage.java:
##########
@@ -141,4 +137,75 @@ public File getStateMachineDir() {
public File getSnapshotDir(String snapshotMetadata) {
return new File(stateMachineDir.getAbsolutePath() + File.separator + snapshotMetadata);
}
+
+ /**
+ * Currently, we name the snapshotDir with Term_Index so that we can tell which directory contains
+ * the latest snapshot files. Unfortunately, when leader install snapshot to a slow follower,
+ * current Ratis implementation will flatten the directory and place all the snapshots directly
+ * under statemachine dir. Under this scenario, we cannot restore Term_Index from directory name.
+ * We decided to add an empty metadata file containing only Term_Index into the snapshotDir. his
+ * metadata file will be installed along with application snapshot files, so that Term_Index
+ * information is kept during InstallSnapshot.
+ */
+ public boolean addTermIndexMetaFile(File snapshotDir, String termIndexMetadata) {
+ File snapshotMetaFile = new File(getMetafilePath(snapshotDir, termIndexMetadata));
+ try {
+ return snapshotMetaFile.createNewFile();
+ } catch (IOException e) {
+ logger.warn("cannot create snapshot metafile: ", e);
+ return false;
+ }
+ }
+
+ private String getMetafilePath(File snapshotDir, String termIndexMetadata) {
+ // e.g. /_sm/3_39/.ratis_meta.3_39
+ return snapshotDir.getAbsolutePath() + File.separator + META_FILE_PREFIX + termIndexMetadata;
+ }
+
+ private String getMetafileMatcherRegex() {
+ // meta file should always end with term_index
+ return META_FILE_PREFIX + "\\d+_\\d+$";
+ }
+
+ /**
+ * After leader InstallSnapshot to a slow follower, Ratis will put all snapshot files directly
+ * under statemachineDir. We need to handle this special scenario and rearrange these files to
+ * appropriate sub-directory this function will move all snapshot files directly under /sm to
+ * /sm/term_index/
+ */
+ void rearrangeSMDirHierarchyIfNecessary() {
+ File[] potentialMetafile =
+ stateMachineDir.listFiles((dir, name) -> name.matches(getMetafileMatcherRegex()));
+ if (potentialMetafile == null || potentialMetafile.length == 0) {
+ // the statemachine dir contains no direct metafile
+ return;
+ }
+ String metadata = potentialMetafile[0].getName().substring(META_FILE_PREFIX.length());
+
+ File snapshotDir = getSnapshotDir(metadata);
+ snapshotDir.mkdir();
+
+ File[] snapshotFiles = stateMachineDir.listFiles();
+
+ // move files to snapshotDir, if an error occurred, delete snapshotDir
+ try {
+ if (snapshotFiles == null) {
+ logger.error(
+ "An unexpected condition triggered. please check implementation "
+ + this.getClass().getName());
+ FileUtils.deleteFully(snapshotDir);
+ return;
+ }
+
+ for (File file : snapshotFiles) {
+ boolean success = file.renameTo(new File(snapshotDir + File.separator + file.getName()));
+ if (!success) {
+ FileUtils.deleteFully(snapshotDir);
Review Comment:
logger
--
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] SzyWilliam commented on a diff in pull request #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #5991:
URL: https://github.com/apache/iotdb/pull/5991#discussion_r881197665
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/ApplicationStateMachineProxy.java:
##########
@@ -142,13 +144,22 @@ public long takeSnapshot() throws IOException {
// require the application statemachine to take the latest snapshot
String metadata = Utils.getMetadataFromTermIndex(lastApplied);
File snapshotDir = snapshotStorage.getSnapshotDir(metadata);
+
+ // delete snapshotDir fully in case of last failed take snapshot
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] SzyWilliam commented on a diff in pull request #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #5991:
URL: https://github.com/apache/iotdb/pull/5991#discussion_r881199628
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/SnapshotStorage.java:
##########
@@ -141,4 +137,75 @@ public File getStateMachineDir() {
public File getSnapshotDir(String snapshotMetadata) {
return new File(stateMachineDir.getAbsolutePath() + File.separator + snapshotMetadata);
}
+
+ /**
+ * Currently, we name the snapshotDir with Term_Index so that we can tell which directory contains
+ * the latest snapshot files. Unfortunately, when leader install snapshot to a slow follower,
+ * current Ratis implementation will flatten the directory and place all the snapshots directly
+ * under statemachine dir. Under this scenario, we cannot restore Term_Index from directory name.
+ * We decided to add an empty metadata file containing only Term_Index into the snapshotDir. his
+ * metadata file will be installed along with application snapshot files, so that Term_Index
+ * information is kept during InstallSnapshot.
+ */
+ public boolean addTermIndexMetaFile(File snapshotDir, String termIndexMetadata) {
+ File snapshotMetaFile = new File(getMetafilePath(snapshotDir, termIndexMetadata));
+ try {
+ return snapshotMetaFile.createNewFile();
+ } catch (IOException e) {
+ logger.warn("cannot create snapshot metafile: ", e);
+ return false;
+ }
+ }
+
+ private String getMetafilePath(File snapshotDir, String termIndexMetadata) {
+ // e.g. /_sm/3_39/.ratis_meta.3_39
+ return snapshotDir.getAbsolutePath() + File.separator + META_FILE_PREFIX + termIndexMetadata;
+ }
+
+ private String getMetafileMatcherRegex() {
+ // meta file should always end with term_index
+ return META_FILE_PREFIX + "\\d+_\\d+$";
+ }
+
+ /**
+ * After leader InstallSnapshot to a slow follower, Ratis will put all snapshot files directly
+ * under statemachineDir. We need to handle this special scenario and rearrange these files to
+ * appropriate sub-directory this function will move all snapshot files directly under /sm to
+ * /sm/term_index/
+ */
+ void rearrangeSMDirHierarchyIfNecessary() {
+ File[] potentialMetafile =
+ stateMachineDir.listFiles((dir, name) -> name.matches(getMetafileMatcherRegex()));
+ if (potentialMetafile == null || potentialMetafile.length == 0) {
+ // the statemachine dir contains no direct metafile
+ return;
+ }
+ String metadata = potentialMetafile[0].getName().substring(META_FILE_PREFIX.length());
+
+ File snapshotDir = getSnapshotDir(metadata);
+ snapshotDir.mkdir();
+
+ File[] snapshotFiles = stateMachineDir.listFiles();
+
+ // move files to snapshotDir, if an error occurred, delete snapshotDir
+ try {
+ if (snapshotFiles == null) {
+ logger.error(
+ "An unexpected condition triggered. please check implementation "
+ + this.getClass().getName());
+ FileUtils.deleteFully(snapshotDir);
+ return;
+ }
+
+ for (File file : snapshotFiles) {
+ boolean success = file.renameTo(new File(snapshotDir + File.separator + file.getName()));
+ if (!success) {
+ FileUtils.deleteFully(snapshotDir);
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] OneSizeFitsQuorum merged pull request #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
Posted by GitBox <gi...@apache.org>.
OneSizeFitsQuorum merged PR #5991:
URL: https://github.com/apache/iotdb/pull/5991
--
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] SzyWilliam commented on a diff in pull request #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #5991:
URL: https://github.com/apache/iotdb/pull/5991#discussion_r881197983
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/ApplicationStateMachineProxy.java:
##########
@@ -142,13 +144,22 @@ public long takeSnapshot() throws IOException {
// require the application statemachine to take the latest snapshot
String metadata = Utils.getMetadataFromTermIndex(lastApplied);
File snapshotDir = snapshotStorage.getSnapshotDir(metadata);
+
+ // delete snapshotDir fully in case of last failed take snapshot
+ FileUtils.deleteFully(snapshotDir);
+
snapshotDir.mkdir();
if (!snapshotDir.isDirectory()) {
logger.error("Unable to create snapshotDir at {}", snapshotDir);
return RaftLog.INVALID_LOG_INDEX;
}
- boolean success = applicationStateMachine.takeSnapshot(snapshotDir);
- if (!success) {
+
+ boolean applicationTakeSnapshotSuccess = applicationStateMachine.takeSnapshot(snapshotDir);
+ boolean addTermIndexMetafileSuccess =
+ snapshotStorage.addTermIndexMetaFile(snapshotDir, metadata);
Review Comment:
If takeSnapshot() failed, the whole `snapshotDir` will be deleted, including this TermIndexMetaFile.
--
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] SzyWilliam commented on a diff in pull request #5991: [IOTDB-3253] Upgrade dependency: Ratis from 2.2.0 to 2.3.0
Posted by GitBox <gi...@apache.org>.
SzyWilliam commented on code in PR #5991:
URL: https://github.com/apache/iotdb/pull/5991#discussion_r881198726
##########
consensus/src/main/java/org/apache/iotdb/consensus/ratis/SnapshotStorage.java:
##########
@@ -141,4 +137,75 @@ public File getStateMachineDir() {
public File getSnapshotDir(String snapshotMetadata) {
return new File(stateMachineDir.getAbsolutePath() + File.separator + snapshotMetadata);
}
+
+ /**
+ * Currently, we name the snapshotDir with Term_Index so that we can tell which directory contains
+ * the latest snapshot files. Unfortunately, when leader install snapshot to a slow follower,
+ * current Ratis implementation will flatten the directory and place all the snapshots directly
+ * under statemachine dir. Under this scenario, we cannot restore Term_Index from directory name.
+ * We decided to add an empty metadata file containing only Term_Index into the snapshotDir. his
+ * metadata file will be installed along with application snapshot files, so that Term_Index
+ * information is kept during InstallSnapshot.
+ */
+ public boolean addTermIndexMetaFile(File snapshotDir, String termIndexMetadata) {
+ File snapshotMetaFile = new File(getMetafilePath(snapshotDir, termIndexMetadata));
+ try {
+ return snapshotMetaFile.createNewFile();
+ } catch (IOException e) {
+ logger.warn("cannot create snapshot metafile: ", e);
+ return false;
+ }
+ }
+
+ private String getMetafilePath(File snapshotDir, String termIndexMetadata) {
+ // e.g. /_sm/3_39/.ratis_meta.3_39
+ return snapshotDir.getAbsolutePath() + File.separator + META_FILE_PREFIX + termIndexMetadata;
+ }
+
+ private String getMetafileMatcherRegex() {
+ // meta file should always end with term_index
+ return META_FILE_PREFIX + "\\d+_\\d+$";
+ }
+
+ /**
+ * After leader InstallSnapshot to a slow follower, Ratis will put all snapshot files directly
+ * under statemachineDir. We need to handle this special scenario and rearrange these files to
+ * appropriate sub-directory this function will move all snapshot files directly under /sm to
+ * /sm/term_index/
+ */
+ void rearrangeSMDirHierarchyIfNecessary() {
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org