fixes #449 fix two bugs with WAL recovery #458

keith-turner · 2018-05-02T19:36:02Z

Fix bug where tablet is unloaded, reloaded on tserver, and then tserver dies
Fix bug with out of order logs. Recovery code assumed logs were passed in
time order. However, since 1.8.0 they have been passed in random order. Rewrote
recovery code to handle out of order logs. The fix was to read all logs in
a sorted merged way.

ctubbsii · 2018-05-02T19:38:34Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/RecoveryLogsIterator.java

+import org.apache.accumulo.tserver.logger.LogFileKey;
+import org.apache.accumulo.tserver.logger.LogFileValue;
+import org.apache.hadoop.fs.Path;
+import org.mortbay.log.Log;


mortbay log is probably not the Log you want

milleruntime · 2018-05-02T20:37:40Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/RecoveryLogsIterator.java

+
+  private static class MultiReaderIterator implements Iterator<Entry<LogFileKey,LogFileValue>> {
+
+    private MultiReader reader;


The MultiReader should be renamed. Then the MultiReaderIterator as well.

Perhaps this Iterator could be moved into "MultiReader"

milleruntime · 2018-05-02T20:44:45Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

-        lastFinish = newFinish;
-    }
+  private int findMaxTabletId(KeyExtent extent, List<Path> recoveryLogs) throws IOException {
+    int tid = -1;


Should use less abstract name than "tid". We use this name for different things throughout the code.

milleruntime · 2018-05-02T20:58:00Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

@@ -16,132 +16,79 @@
 */
 package org.apache.accumulo.tserver.log;

+import static com.google.common.base.Preconditions.checkState;


Should just import the class. I found myself looking around for your definition of checkState. Perhaps this isn't a big of deal in an IDE.

milleruntime · 2018-05-02T21:02:47Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

  }

-  private void playbackMutations(MultiReader reader, int tid, LastStartToFinish lastStartToFinish,
+  public void recover(KeyExtent extent, List<Path> recoveryLogs, Set<String> tabletFiles,


Personal preference to have public methods and entry points at the top of classes. Unlike binary data, humans read from the top down :)

milleruntime · 2018-05-02T21:10:42Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

+    try (RecoveryLogsIterator rli = new RecoveryLogsIterator(fs, recoveryLogs, COMPACTION_START,
+        tid)) {
+
+      DeduplicatingIterator ddi = new DeduplicatingIterator(rli);


Could DeduplicatingIterator be Autoclosable as well?

If not, looks like you need to close it.

milleruntime · 2018-05-02T21:14:38Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

+    return recoverySeq;
+  }
+
+  private void playbackMutations(List<Path> recoveryLogs, MutationReceiver mr, int tid,


Nice improvement... this method now becomes a lot more clear.

mikewalch · 2018-05-02T21:23:03Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/RecoveryLogsIterator.java

+  private List<MultiReader> readers;
+  private UnmodifiableIterator<Entry<LogFileKey,LogFileValue>> iter;
+
+  private static class MultiReaderIterator implements Iterator<Entry<LogFileKey,LogFileValue>> {


Default method 'remove' should be overridden

mikewalch · 2018-05-02T21:23:22Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/RecoveryLogsIterator.java

+    }
+  }
+
+  static class SortCheckIterator implements Iterator<Entry<LogFileKey,LogFileValue>> {


Default method 'remove' should be overridden

mikewalch · 2018-05-02T21:24:03Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/RecoveryLogsIterator.java

+import static org.apache.accumulo.tserver.logger.LogEvents.OPEN;
+
+import java.io.IOException;
+import java.io.UncheckedIOException;


This is a Java 8 exception

mikewalch · 2018-05-02T21:24:43Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

-  int findLastStartToFinish(MultiReader reader, int fileno, KeyExtent extent,
-      Set<String> tabletFiles, LastStartToFinish lastStartToFinish)
-      throws IOException, EmptyMapFileException, UnusedException {
+  static class DeduplicatingIterator implements Iterator<Entry<LogFileKey,LogFileValue>> {


Default method 'remove' should be overridden

ctubbsii · 2018-05-03T21:11:35Z

server/tserver/src/main/java/org/apache/accumulo/tserver/log/SortedLogRecovery.java

@@ -106,12 +106,12 @@ private int findMaxTabletId(KeyExtent extent, List<Path> recoveryLogs) throws IO
        checkState(key.event == DEFINE_TABLET); // should only fail if bug elsewhere

        if (key.tablet.equals(extent) || key.tablet.equals(alternative)) {
-          checkState(key.tid >= 0, "Tid %s for %s is negative", key.tid, extent);
-          checkState(tabletId == -1 || key.tid >= tabletId); // should only fail if bug in
+          checkState(key.tabletId >= 0, "Tid %s for %s is negative", key.tabletId, extent);


Could update the log messages to refer to "tabletId" instead of "Tid", too.

mikewalch

It looks like a test is failing...

https://jenkins.revelc.net/job/Accumulo-PR-Build/73/

keith-turner · 2018-05-07T14:31:17Z

Using these changes, a ~24hr test of CI w/ agitation completed with no data loss. There were 8 nodes runnning tservers and tservers were killed 279 times during the test.

        org.apache.accumulo.test.continuous.ContinuousVerify$Counts
                REFERENCED=24169811377
                UNREFERENCED=7532412

* Fix bug where tablet is unloaded, reloaded on tserver, and then tserver dies * Fix bug with out of order logs. Recovery code assumed logs were passed in time order. However, since 1.8.0 they have been passed in random order. Rewrote recovery code to handle out of order logs. The fix was to read all logs in a sorted merged way.

ctubbsii reviewed May 2, 2018

View reviewed changes

milleruntime reviewed May 2, 2018

View reviewed changes

mikewalch reviewed May 2, 2018

View reviewed changes

ctubbsii added a commit to ctubbsii/accumulo that referenced this pull request May 2, 2018

Suggested updates to apache#458 pull request

1f9af30

ctubbsii reviewed May 3, 2018

View reviewed changes

mikewalch reviewed May 4, 2018

View reviewed changes

milleruntime approved these changes May 7, 2018

View reviewed changes

keith-turner mentioned this pull request May 7, 2018

-1 tablet log id used in WAL when table durability is set to None #477

Closed

ctubbsii approved these changes May 8, 2018

View reviewed changes

mikewalch approved these changes May 8, 2018

View reviewed changes

keith-turner and others added 4 commits May 8, 2018 11:31

Fixes apache#449 suggested updates to PR apache#458

3478605

fixes apache#449 code review updates (apache#458)

36ec18d

fixes apache#477 Stop putting -1 in WAL (apache#458)

e38b88e

keith-turner force-pushed the accumulo-449 branch from e41f0ad to e38b88e Compare May 8, 2018 15:32

keith-turner merged commit b8f574f into apache:1.9 May 8, 2018

keith-turner pushed a commit that referenced this pull request May 8, 2018

Fixes #449 suggested updates to PR #458

e56340c

keith-turner added a commit that referenced this pull request May 8, 2018

fixes #449 code review updates (#458)

a8fead2

keith-turner deleted the accumulo-449 branch May 8, 2018 15:36

keith-turner added v2.0.0 bug This issue has been verified to be a bug. labels May 8, 2018

keith-turner added a commit that referenced this pull request May 8, 2018

fix build problems from #458

be7475e

adamjshook mentioned this pull request Jun 20, 2018

WAL recovery enhancements and tooling #535

Closed

keith-turner mentioned this pull request Jan 8, 2019

Look into reusing open map files during log recovery. #882

Open

ctubbsii added this to Done in 1.9.1 Jun 14, 2019

ctubbsii added this to Done in 2.0.0 Jun 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #449 fix two bugs with WAL recovery #458

fixes #449 fix two bugs with WAL recovery #458

keith-turner commented May 2, 2018

ctubbsii May 2, 2018

milleruntime May 2, 2018 •

edited

Loading

milleruntime May 2, 2018

milleruntime May 2, 2018

milleruntime May 2, 2018

milleruntime May 2, 2018

milleruntime May 2, 2018

milleruntime May 2, 2018

milleruntime May 2, 2018

mikewalch May 2, 2018

mikewalch May 2, 2018

mikewalch May 2, 2018

mikewalch May 2, 2018

ctubbsii May 3, 2018

mikewalch left a comment

keith-turner commented May 7, 2018


		private static class MultiReaderIterator implements Iterator<Entry<LogFileKey,LogFileValue>> {

		private MultiReader reader;

fixes #449 fix two bugs with WAL recovery #458

fixes #449 fix two bugs with WAL recovery #458

Conversation

keith-turner commented May 2, 2018

Choose a reason for hiding this comment

milleruntime May 2, 2018 • edited Loading

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

mikewalch left a comment

Choose a reason for hiding this comment

keith-turner commented May 7, 2018

milleruntime May 2, 2018 •

edited

Loading