This is an automated email from the ASF dual-hosted git repository.
kturner pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new 7efee59b45 Revert "acquires two locks in correct order in tablet close
(#4751)"
7efee59b45 is described below
commit 7efee59b45ccfd5010a45e30b148f6031b368417
Author: Keith Turner <[email protected]>
AuthorDate: Thu Jul 25 15:03:33 2024 -0400
Revert "acquires two locks in correct order in tablet close (#4751)"
This reverts commit dd40162b19b3d75325e7fd873221ad055d2b637f.
---
.../accumulo/tserver/tablet/DatafileManager.java | 6 +-
.../org/apache/accumulo/tserver/tablet/Tablet.java | 111 +++++++++------------
2 files changed, 50 insertions(+), 67 deletions(-)
diff --git
a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
index 4697439421..b2568f7ade 100644
---
a/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
+++
b/server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
@@ -348,7 +348,9 @@ class DatafileManager {
metadataUpdateCount.updateAndGet(MetadataUpdateCount::incrementStart);
// do not place any code here between above stmt and following try{}finally
try {
- final var logLock = tablet.lockLogLock();
+ // Should not hold the tablet lock while trying to acquire the log lock
because this could
+ // lead to deadlock. However there is a path in the code that does this.
See #3759
+ tablet.getLogLock().lock();
// do not place any code here between lock and try
try {
// The following call pairs with tablet.finishClearingUnusedLogs()
later in this block. If
@@ -403,7 +405,7 @@ class DatafileManager {
tablet.finishClearingUnusedLogs();
} finally {
- logLock.unlock();
+ tablet.getLogLock().unlock();
}
do {
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 544b6fc60a..b6ad6150cb 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
@@ -903,12 +903,7 @@ public class Tablet extends TabletBase {
@Override
public void close(boolean saveState) throws IOException {
initiateClose(saveState);
- final var lock = lockLogLock();
- try {
- completeClose(saveState, true);
- } finally {
- lock.unlock();
- }
+ completeClose(saveState, true);
log.info("Tablet {} closed.", this.extent);
}
@@ -1024,11 +1019,6 @@ public class Tablet extends TabletBase {
synchronized void completeClose(boolean saveState, boolean completeClose)
throws IOException {
- // The lockLock must be acquired before the tablet lock. Later in this
function the log lock may
- // be acquired during minor compaction. It will fail if the tablet lock is
held and not the log
- // lock. Fail sooner here and always, not only in the case when a minor
compaction is needed.
- Preconditions.checkState(logLock.isHeldByCurrentThread());
-
if (!isClosing() || isCloseComplete() || closeCompleting) {
throw new IllegalStateException("Bad close state " + closeState + " on
tablet " + extent);
}
@@ -1641,68 +1631,61 @@ public class Tablet extends TabletBase {
Map<TabletFile,FileUtil.FileInfo> firstAndLastRows =
FileUtil.tryToGetFirstAndLastRows(context,
tableConfiguration, getDatafileManager().getFiles());
- // This must be acquired before the tablet lock AND completeClose expects
it to be acquired when
- // called.
- final var lock = lockLogLock();
- try {
- synchronized (this) {
- // java needs tuples ...
- TreeMap<KeyExtent,TabletData> newTablets = new TreeMap<>();
+ synchronized (this) {
+ // java needs tuples ...
+ TreeMap<KeyExtent,TabletData> newTablets = new TreeMap<>();
- long t1 = System.currentTimeMillis();
+ long t1 = System.currentTimeMillis();
- closeState = CloseState.CLOSING;
- completeClose(true, false);
+ closeState = CloseState.CLOSING;
+ completeClose(true, false);
- Text midRow = splitPoint.row;
- double splitRatio = splitPoint.splitRatio;
+ Text midRow = splitPoint.row;
+ double splitRatio = splitPoint.splitRatio;
- KeyExtent low = new KeyExtent(extent.tableId(), midRow,
extent.prevEndRow());
- KeyExtent high = new KeyExtent(extent.tableId(), extent.endRow(),
midRow);
+ KeyExtent low = new KeyExtent(extent.tableId(), midRow,
extent.prevEndRow());
+ KeyExtent high = new KeyExtent(extent.tableId(), extent.endRow(),
midRow);
- String lowDirectoryName = createTabletDirectoryName(context, midRow);
+ String lowDirectoryName = createTabletDirectoryName(context, midRow);
- // write new tablet information to MetadataTable
- SortedMap<StoredTabletFile,DataFileValue> lowDatafileSizes = new
TreeMap<>();
- SortedMap<StoredTabletFile,DataFileValue> highDatafileSizes = new
TreeMap<>();
- List<StoredTabletFile> highDatafilesToRemove = new ArrayList<>();
+ // write new tablet information to MetadataTable
+ SortedMap<StoredTabletFile,DataFileValue> lowDatafileSizes = new
TreeMap<>();
+ SortedMap<StoredTabletFile,DataFileValue> highDatafileSizes = new
TreeMap<>();
+ List<StoredTabletFile> highDatafilesToRemove = new ArrayList<>();
- MetadataTableUtil.splitDatafiles(midRow, splitRatio, firstAndLastRows,
- getDatafileManager().getDatafileSizes(), lowDatafileSizes,
highDatafileSizes,
- highDatafilesToRemove);
+ MetadataTableUtil.splitDatafiles(midRow, splitRatio, firstAndLastRows,
+ getDatafileManager().getDatafileSizes(), lowDatafileSizes,
highDatafileSizes,
+ highDatafilesToRemove);
- log.debug("Files for low split {} {}", low, lowDatafileSizes.keySet());
- log.debug("Files for high split {} {}", high,
highDatafileSizes.keySet());
+ log.debug("Files for low split {} {}", low, lowDatafileSizes.keySet());
+ log.debug("Files for high split {} {}", high,
highDatafileSizes.keySet());
- MetadataTime time = tabletTime.getMetadataTime();
+ MetadataTime time = tabletTime.getMetadataTime();
- HashSet<ExternalCompactionId> ecids = new HashSet<>();
- compactable.getExternalCompactionIds(ecids::add);
+ HashSet<ExternalCompactionId> ecids = new HashSet<>();
+ compactable.getExternalCompactionIds(ecids::add);
- MetadataTableUtil.splitTablet(high, extent.prevEndRow(), splitRatio,
- getTabletServer().getContext(), getTabletServer().getLock(),
ecids);
- ManagerMetadataUtil.addNewTablet(getTabletServer().getContext(), low,
lowDirectoryName,
- getTabletServer().getTabletSession(), lowDatafileSizes,
bulkImported, time,
- lastFlushID.get(), lastCompactID.get(),
getTabletServer().getLock());
- MetadataTableUtil.finishSplit(high, highDatafileSizes,
highDatafilesToRemove,
- getTabletServer().getContext(), getTabletServer().getLock());
+ MetadataTableUtil.splitTablet(high, extent.prevEndRow(), splitRatio,
+ getTabletServer().getContext(), getTabletServer().getLock(), ecids);
+ ManagerMetadataUtil.addNewTablet(getTabletServer().getContext(), low,
lowDirectoryName,
+ getTabletServer().getTabletSession(), lowDatafileSizes,
bulkImported, time,
+ lastFlushID.get(), lastCompactID.get(), getTabletServer().getLock());
+ MetadataTableUtil.finishSplit(high, highDatafileSizes,
highDatafilesToRemove,
+ getTabletServer().getContext(), getTabletServer().getLock());
- TabletLogger.split(extent, low, high,
getTabletServer().getTabletSession());
+ TabletLogger.split(extent, low, high,
getTabletServer().getTabletSession());
- newTablets.put(high, new TabletData(dirName, highDatafileSizes, time,
lastFlushID.get(),
- lastCompactID.get(), lastLocation, bulkImported));
- newTablets.put(low, new TabletData(lowDirectoryName, lowDatafileSizes,
time,
- lastFlushID.get(), lastCompactID.get(), lastLocation,
bulkImported));
+ newTablets.put(high, new TabletData(dirName, highDatafileSizes, time,
lastFlushID.get(),
+ lastCompactID.get(), lastLocation, bulkImported));
+ newTablets.put(low, new TabletData(lowDirectoryName, lowDatafileSizes,
time,
+ lastFlushID.get(), lastCompactID.get(), lastLocation, bulkImported));
- long t2 = System.currentTimeMillis();
+ long t2 = System.currentTimeMillis();
- log.debug(String.format("offline split time : %6.2f secs", (t2 - t1) /
1000.0));
+ log.debug(String.format("offline split time : %6.2f secs", (t2 - t1) /
1000.0));
- closeState = CloseState.COMPLETE;
- return newTablets;
- }
- } finally {
- lock.unlock();
+ closeState = CloseState.COMPLETE;
+ return newTablets;
}
}
@@ -1945,12 +1928,7 @@ public class Tablet extends TabletBase {
}
}
- ReentrantLock lockLogLock() {
- // It is expected that the log lock is acquired before the tablet lock. If
they are acquired in
- // reverse order it could lead to deadlock. So if the current thread does
not hold the log lock,
- // then it should also not hold the tablet lock.
- Preconditions.checkState(!Thread.holdsLock(this) ||
logLock.isHeldByCurrentThread());
- logLock.lock();
+ ReentrantLock getLogLock() {
return logLock;
}
@@ -2018,7 +1996,10 @@ public class Tablet extends TabletBase {
boolean releaseLock = true;
- final var lock = lockLogLock();
+ // Should not hold the tablet lock while trying to acquire the log lock
because this could lead
+ // to deadlock. However there is a path in the code that does this. See
#3759
+ logLock.lock();
+
try {
synchronized (this) {
@@ -2074,7 +2055,7 @@ public class Tablet extends TabletBase {
}
} finally {
if (releaseLock) {
- lock.unlock();
+ logLock.unlock();
}
}
}
