[ALLUXIO-2408] Fix potential journal checkpoint failure #4306

aaudiber · 2016-11-03T22:23:31Z

https://alluxio.atlassian.net/browse/ALLUXIO-2408

AmplabJenkins · 2016-11-03T22:45:07Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-11-03T22:45:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12180/
Test PASSed.

alluxio-bot · 2016-11-03T22:45:09Z

Running checks...

Valid pull request title: true
Contains JIRA link: true
Commits associated with Github account: true

All checks passed!

AmplabJenkins · 2016-11-04T01:26:39Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-11-04T01:26:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12184/
Test PASSed.

gpang

@aaudiber Thanks for this fix! I left a few comments.

gpang · 2016-11-04T13:57:34Z

core/common/src/main/java/alluxio/util/UnderFileSystemUtils.java

+        ufs.delete(path, false);
+      }
+    } catch (IOException e) {
+      throw Throwables.propagate(e);


Which exceptions will be propagated? Technically, even if you check for exists before you delete, there is no guarantee it will still exist at the time of the delete. Therefore, there is currently no way to do this atomically.

So, when should this method not throw an error, and when should it? What is the purpose/goal for this method?

The javadoc for UnderFileSystem just says @throws IOException if a non-Alluxio error occurs for both exists and delete. This method is never expected to throw an exception.

I don't think the lack of atomicity is a problem. The contract is that if the file path exists, it gets deleted. If it gets deleted between the exists and the delete, the delete will return false and the result is the same (no more file)

gpang · 2016-11-04T13:59:21Z

core/server/src/main/java/alluxio/master/AbstractMaster.java

@@ -118,6 +118,8 @@ public void start(boolean isLeader) throws IOException {
       * concurrent access to the master during these phases.
       */

+      mJournalWriter.recoverCheckpoint();


Should this be commented on in the previous comments about the steps/phases?

gpang · 2016-11-04T14:00:57Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

+  /** Absolute path to the backup checkpoint file. */
+  private final String mBackupCheckpointPath;
+  /** Absolute path to the temporary backup checkpoint file. */
+  private final String mTempBackupCheckpointPath;


hrmmm, maybe there should be a javadoc somewhere that describes this process. There is a temp, backup, and tempbackup file, which can be confusing.

I've added more detailed javadocs for each of these

gpang · 2016-11-04T14:03:12Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

+  public synchronized void recoverCheckpoint() {
+    try {
+      if (mUfs.exists(mBackupCheckpointPath)) {
+        if (mUfs.exists(mJournal.getCheckpointFilePath())) {


should mJournal.getCheckpointFilePath() be mCheckpointPath?

fixed, thanks

gpang · 2016-11-04T14:05:37Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

+          deleteCompletedLogs();
+          mUfs.delete(mBackupCheckpointPath, false);
+        } else {
+          // We must have crashed before writing the checkpoint file, restore the checkpoint from


I'm not sure when this would happen, so if there is a javadoc for this class that describes the checkpointing process, that would be helpful.

I've added more docs here

gpang · 2016-11-04T14:07:44Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

@@ -179,16 +213,15 @@ private synchronized OutputStream openCurrentLog() throws IOException {
   */
  private synchronized void deleteCompletedLogs() throws IOException {
    LOG.info("Deleting all completed log files...");
-    // Loop over all complete logs starting from the beginning.
-    // TODO(gpang): should the deletes start from the end?


Do you think this is still relevant? I think I put in this comment since we do not delete completed logs "atomically". However, with your change, it seems like the deletion of completed logs can look atomic now? if something bad happens during the deletion, it will continue the deletion on the next startup, right?

this method finds the logs by seeing how high it can count before the log of that number doesn't exist. If we have 10 logs and only delete log #1, the next time we try to delete the logs we won't delete logs 2-10 because we see that log 1 doesn't exist.

ahhh, I see. I guess the TODO was correct. Thanks for fixing it.

gpang · 2016-11-04T14:08:17Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

@@ -274,16 +307,23 @@ public synchronized void close() throws IOException {
      mOutputStream.close();

      LOG.info("Successfully created tmp checkpoint file: {}", mTempCheckpointPath);
-      mUfs.delete(mJournal.getCheckpointFilePath(), false);
-      // TODO(gpang): the real checkpoint should not be overwritten here, but after all operations.


Thanks for fixing this!

aaudiber · 2016-11-04T17:24:35Z

Thanks for the review @gpang, I think it's much better now that I've added more comments/javadoc. All of your comments should be addressed now, PTAL

AmplabJenkins · 2016-11-04T17:42:00Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-11-04T17:42:00Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12188/
Test PASSed.

gpang

@aaudiber Thanks! Left a few comments, and looks good.

gpang · 2016-11-04T20:29:28Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

+  private final String mBackupCheckpointPath;
+  /**
+   * Absolute path to the temporary backup checkpoint file. This path is used as an intermediate
+   * rename step when backing up mCheckpointPath to mBackupCheckpointPath.


NIT: use {@link } for mCheckpointPath and mBackupCheckpointPath

gpang · 2016-11-04T21:00:25Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

+        UnderFileSystemUtils.deleteIfExists(mUfs, mTempBackupCheckpointPath);
+        UnderFileSystemUtils.deleteIfExists(mUfs, mBackupCheckpointPath);
+        // Rename in two steps so that we never have identical mCheckpointPath and
+        // mBackupCheckpointPath. This is a concern since UFS may implement rename as copy + delete.


This seems like an invariant we want to maintain. Maybe this invariant should be explicitly mentioned in a class javadoc?

Added more docs and centralized the code in one place to make it easier to see all the invariants

gpang · 2016-11-04T21:01:01Z

core/server/src/main/java/alluxio/master/journal/JournalWriter.java

+  public synchronized void recoverCheckpoint() {
+    try {
+      if (mUfs.exists(mTempBackupCheckpointPath)) {
+        // If mCheckpointPath exists, step 2 must have implemented rename as copy + delete, and


This is assuming the invariant that both dont exist at the same time. Do we want to precondition that, or is that potentially unsafe?

The only assumption is that all three files don't exist. I've added a Precondition for this

AmplabJenkins · 2016-11-07T18:51:33Z

Merged build finished. Test FAILed.

AmplabJenkins · 2016-11-07T18:51:34Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12201/

Build result: FAILURE

[...truncated 1887 lines...][JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/shell/pom.xml to org.alluxio/alluxio-shell/1.4.0-SNAPSHOT/alluxio-shell-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/oss/pom.xml to org.alluxio/alluxio-underfs-oss/1.4.0-SNAPSHOT/alluxio-underfs-oss-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/oss/target/alluxio-underfs-oss-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-oss/1.4.0-SNAPSHOT/alluxio-underfs-oss-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/oss/target/alluxio-underfs-oss-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-oss/1.4.0-SNAPSHOT/alluxio-underfs-oss-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/keyvalue/client/pom.xml to org.alluxio/alluxio-keyvalue-client/1.4.0-SNAPSHOT/alluxio-keyvalue-client-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/hdfs/pom.xml to org.alluxio/alluxio-underfs-hdfs/1.4.0-SNAPSHOT/alluxio-underfs-hdfs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/hdfs/target/alluxio-underfs-hdfs-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-hdfs/1.4.0-SNAPSHOT/alluxio-underfs-hdfs-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/hdfs/target/alluxio-underfs-hdfs-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-hdfs/1.4.0-SNAPSHOT/alluxio-underfs-hdfs-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/swift/pom.xml to org.alluxio/alluxio-underfs-swift/1.4.0-SNAPSHOT/alluxio-underfs-swift-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/swift/target/alluxio-underfs-swift-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-swift/1.4.0-SNAPSHOT/alluxio-underfs-swift-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/swift/target/alluxio-underfs-swift-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-swift/1.4.0-SNAPSHOT/alluxio-underfs-swift-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/s3a/pom.xml to org.alluxio/alluxio-underfs-s3a/1.4.0-SNAPSHOT/alluxio-underfs-s3a-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/s3a/target/alluxio-underfs-s3a-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-s3a/1.4.0-SNAPSHOT/alluxio-underfs-s3a-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/s3a/target/alluxio-underfs-s3a-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-s3a/1.4.0-SNAPSHOT/alluxio-underfs-s3a-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/examples/pom.xml to org.alluxio/alluxio-examples/1.4.0-SNAPSHOT/alluxio-examples-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/core/server/pom.xml to org.alluxio/alluxio-core-server/1.4.0-SNAPSHOT/alluxio-core-server-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/keyvalue/common/pom.xml to org.alluxio/alluxio-keyvalue-common/1.4.0-SNAPSHOT/alluxio-keyvalue-common-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/pom.xml to org.alluxio/alluxio-underfs/1.4.0-SNAPSHOT/alluxio-underfs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/glusterfs/pom.xml to org.alluxio/alluxio-underfs-glusterfs/1.4.0-SNAPSHOT/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/glusterfs/target/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT.jar to org.alluxio/alluxio-underfs-glusterfs/1.4.0-SNAPSHOT/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/underfs/glusterfs/target/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT-sources.jar to org.alluxio/alluxio-underfs-glusterfs/1.4.0-SNAPSHOT/alluxio-underfs-glusterfs-1.4.0-SNAPSHOT-sources.jar[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/integration/pom.xml to org.alluxio/alluxio-integration/1.4.0-SNAPSHOT/alluxio-integration-1.4.0-SNAPSHOT.pom[JENKINS] Archiving /home/jenkins/workspace/Alluxio-Pull-Request-Builder/keyvalue/pom.xml to org.alluxio/alluxio-keyvalue/1.4.0-SNAPSHOT/alluxio-keyvalue-1.4.0-SNAPSHOT.pomchannel stopped
Test FAILed.

gpang

@aaudiber some minor comments

gpang · 2016-11-07T19:46:03Z

core/server/src/main/java/alluxio/master/journal/CheckpointManager.java

+   * Creates a new instance of {@link CheckpointManager}.
+   *
+   * @param ufs the under file system holding the journal
+   * @param the directory for the journal


NIT: the javadoc params seem out of date with the actual parameters of the constructor.

gpang · 2016-11-07T19:48:13Z

core/server/src/main/java/alluxio/master/journal/CheckpointManager.java

+        // Rename in two steps so that we never have identical mCheckpointPath and
+        // mBackupCheckpointPath. This is a concern since UFS may implement rename as copy + delete.
+        mUfs.rename(mCheckpointPath, mTempBackupCheckpointPath);
+        mUfs.rename(mTempBackupCheckpointPath, mBackupCheckpointPath);


NIT: maybe add a log message after this that the backup of the checkpoint was completed?

gpang · 2016-11-07T19:50:24Z

core/server/src/main/java/alluxio/master/journal/CheckpointManager.java

+  public void recoverCheckpoint() {
+    try {
+      Preconditions.checkState(
+          !(mUfs.exists(mCheckpointPath) && mUfs.exists(mTempBackupCheckpointPath)


Will there be duplicate exists calls for these files? Maybe add a TODO to remove the duplicate calls?

Since we cannot guarantee atomicity between exists & next operation, I think we should just cache the values for the duration of the recovery, unless we expect an update.

AmplabJenkins · 2016-11-07T20:11:42Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-11-07T20:11:42Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12203/
Test PASSed.

calvinjia · 2016-11-07T22:30:37Z

core/server/src/main/java/alluxio/master/journal/CheckpointManager.java

+  public void recoverCheckpoint() {
+    try {
+      Preconditions.checkState(
+          !(mUfs.exists(mCheckpointPath) && mUfs.exists(mTempBackupCheckpointPath)


Since we cannot guarantee atomicity between exists & next operation, I think we should just cache the values for the duration of the recovery, unless we expect an update.

calvinjia · 2016-11-07T22:32:20Z

core/server/src/main/java/alluxio/master/journal/CheckpointManager.java

+  /**
+   * Recovers the checkpoint file in case the master crashed while updating it previously.
+   */
+  public void recoverCheckpoint() {


Could you document the expected state after this method is called? ie.
CheckpointPath is always valid, CheckpointPath + editlogs -> up to date master state?

aaudiber · 2016-11-08T18:14:49Z

@calvinjia @gpang All comments addressed, PTAL

calvinjia · 2016-11-08T18:30:13Z

LGTM

gpang

LGTM

AmplabJenkins · 2016-11-08T18:37:08Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-11-08T18:37:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12215/
Test PASSed.

AmplabJenkins · 2016-11-08T18:38:39Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-11-08T18:38:40Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/12214/
Test PASSed.

aaudiber added 2 commits November 3, 2016 12:57

Fix checkpoint error case

ad66225

Add tests for failure during checkpointing

e33873f

aaudiber changed the title ~~[ALLUXIO-2408] Journal checkpoint failure~~ [ALLUXIO-2408] Fix potential journal checkpoint failure Nov 3, 2016

Doc improvements

b50235d

gpang suggested changes Nov 4, 2016

View reviewed changes

aaudiber added 2 commits November 4, 2016 10:17

Add more docs and refactor

7b5c785

Add more recoverCheckpoint comments

2ed0c2e

gpang reviewed Nov 4, 2016

View reviewed changes

aaudiber added 2 commits November 4, 2016 15:38

Refactor journal checkpointing code to its own class

deb1ee1

Update javadoc

c0a9908

gpang reviewed Nov 7, 2016

View reviewed changes

Fix checkstyle

d731c30

calvinjia requested changes Nov 7, 2016

View reviewed changes

aaudiber added 2 commits November 8, 2016 10:09

Factor out checking for checkpoint existence

04b690f

Add additional javadoc

bf2bf4e

calvinjia approved these changes Nov 8, 2016

View reviewed changes

gpang approved these changes Nov 8, 2016

View reviewed changes

aaudiber merged commit d621807 into Alluxio:master Nov 8, 2016

aaudiber deleted the journal-checkpoint-failure branch January 20, 2017 19:26

[ALLUXIO-2408] Fix potential journal checkpoint failure #4306

[ALLUXIO-2408] Fix potential journal checkpoint failure #4306

Conversation

aaudiber commented Nov 3, 2016

AmplabJenkins commented Nov 3, 2016

AmplabJenkins commented Nov 3, 2016

alluxio-bot commented Nov 3, 2016

AmplabJenkins commented Nov 4, 2016

AmplabJenkins commented Nov 4, 2016

gpang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaudiber commented Nov 4, 2016

AmplabJenkins commented Nov 4, 2016

AmplabJenkins commented Nov 4, 2016

gpang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Nov 7, 2016

AmplabJenkins commented Nov 7, 2016

Build result: FAILURE

gpang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Nov 7, 2016

AmplabJenkins commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaudiber commented Nov 8, 2016

calvinjia commented Nov 8, 2016

gpang left a comment

Choose a reason for hiding this comment

AmplabJenkins commented Nov 8, 2016

AmplabJenkins commented Nov 8, 2016

AmplabJenkins commented Nov 8, 2016

AmplabJenkins commented Nov 8, 2016