keith-turner closed pull request #559: fixes #558 use copy to avoid deadlock in
tserver
URL: https://github.com/apache/accumulo/pull/559
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
index 459087248e..4dc19f293d 100644
---
a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
+++
b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java
@@ -154,8 +154,11 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
+import com.google.common.base.Preconditions;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
/**
*
@@ -495,6 +498,8 @@ public void receive(Mutation m) {
logEntry.getColumnQualifier().toString()));
}
+ rebuildReferencedLogs();
+
log.info(
"Write-Ahead Log recovery complete for " + this.extent + " (" +
entriesUsedOnTablet.get()
+ " mutations applied, " + getTabletMemory().getNumEntries() + "
entries created)");
@@ -950,6 +955,8 @@ DataFileValue minorCompact(VolumeManager fs, InMemoryMap
memTable, FileRef tmpDa
private synchronized MinorCompactionTask prepareForMinC(long flushId,
MinorCompactionReason mincReason) {
+ Preconditions.checkState(otherLogs.isEmpty());
+ Preconditions.checkState(referencedLogs.equals(currentLogs));
CommitSession oldCommitSession = getTabletMemory().prepareForMinC();
otherLogs = currentLogs;
currentLogs = new HashSet<>();
@@ -1524,9 +1531,9 @@ private void closeConsistencyCheck() {
}
- if (otherLogs.size() != 0 || currentLogs.size() != 0) {
+ if (otherLogs.size() != 0 || currentLogs.size() != 0 ||
referencedLogs.size() != 0) {
String msg = "Closed tablet " + extent + " has walog entries in memory
currentLogs = "
- + currentLogs + " otherLogs = " + otherLogs;
+ + currentLogs + " otherLogs = " + otherLogs + " refererncedLogs = "
+ referencedLogs;
log.error(msg);
throw new RuntimeException(msg);
}
@@ -2457,19 +2464,25 @@ public void importMapFiles(long tid,
Map<FileRef,MapFileInfo> fileMap, boolean s
}
private Set<DfsLogger> currentLogs = new HashSet<>();
+ private Set<DfsLogger> otherLogs = Collections.emptySet();
+
+ // An immutable copy of currentLogs + otherLogs. This exists so that
removeInUseLogs() does not
+ // have to get the tablet lock. See #558
+ private volatile Set<DfsLogger> referencedLogs = Collections.emptySet();
+
+ private synchronized void rebuildReferencedLogs() {
+ Builder<DfsLogger> builder = ImmutableSet.builder();
+ builder.addAll(currentLogs);
+ builder.addAll(otherLogs);
+ referencedLogs = builder.build();
+ }
public void removeInUseLogs(Set<DfsLogger> candidates) {
- synchronized (this) {
- // remove logs related to minor compacting data
- candidates.removeAll(otherLogs);
- // remove logs related to tablets in memory data
- candidates.removeAll(currentLogs);
- // remove logs related to minor compaction file being added to the
metadata table
- candidates.removeAll(doomedLogs);
- }
+ candidates.removeAll(referencedLogs);
}
Set<String> beginClearingUnusedLogs() {
+ Set<String> unusedLogs = new HashSet<>();
ArrayList<String> otherLogsCopy = new ArrayList<>();
ArrayList<String> currentLogsCopy = new ArrayList<>();
@@ -2478,20 +2491,26 @@ public void removeInUseLogs(Set<DfsLogger> candidates) {
logLock.lock();
synchronized (this) {
- if (doomedLogs.size() > 0)
+ if (removingLogs)
throw new IllegalStateException("Attempted to clear logs when removal
of logs in progress");
for (DfsLogger logger : otherLogs) {
otherLogsCopy.add(logger.toString());
- doomedLogs.add(logger);
+ unusedLogs.add(logger.getMeta());
}
for (DfsLogger logger : currentLogs) {
currentLogsCopy.add(logger.toString());
- doomedLogs.remove(logger);
+ unusedLogs.remove(logger.getMeta());
}
otherLogs = Collections.emptySet();
+ // Do NOT call rebuildReferenedLogs() here as that could cause walogs to
be GCed before the
+ // minc rfile is written to metadata table (see #539). The clearing of
otherLogs is reflected
+ // in refererncedLogs when finishClearingUnusedLogs() calls
rebuildReferenedLogs().
+
+ if (unusedLogs.size() > 0)
+ removingLogs = true;
}
// do debug logging outside tablet lock
@@ -2503,23 +2522,20 @@ public void removeInUseLogs(Set<DfsLogger> candidates) {
log.debug("Logs for current memory: " + getExtent() + " " + logger);
}
- Set<String> doomed = new HashSet<>();
- for (DfsLogger logger : doomedLogs) {
- log.debug("Logs to be destroyed: " + getExtent() + " " +
logger.getMeta());
- doomed.add(logger.getMeta());
+ for (String logger : unusedLogs) {
+ log.debug("Logs to be destroyed: " + getExtent() + " " + logger);
}
- return doomed;
+ return unusedLogs;
}
synchronized void finishClearingUnusedLogs() {
- doomedLogs.clear();
+ removingLogs = false;
+ rebuildReferencedLogs();
logLock.unlock();
}
- private Set<DfsLogger> otherLogs = Collections.emptySet();
- // logs related to minor compaction file being added to the metadata table
- private Set<DfsLogger> doomedLogs = new HashSet<>();
+ private boolean removingLogs = false;
// this lock is basically used to synchronize writing of log info to metadata
private final ReentrantLock logLock = new ReentrantLock();
@@ -2583,6 +2599,10 @@ else if (memTable == getTabletMemory().getMemTable())
numContained++;
}
+ if (numAdded > 0) {
+ rebuildReferencedLogs();
+ }
+
if (numAdded > 0 && numAdded != 1) {
// expect to add all or none
throw new IllegalArgumentException(
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
