This is an automated email from the ASF dual-hosted git repository.
kturner pushed a commit to branch elasticity
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/elasticity by this push:
new c7ae69b881 fixes bug and removes unneeded wait in delete table (#4346)
c7ae69b881 is described below
commit c7ae69b881da00113f2ceef642f5f470818d569d
Author: Keith Turner <ktur...@apache.org>
AuthorDate: Thu Mar 7 13:31:48 2024 -0500
fixes bug and removes unneeded wait in delete table (#4346)
Noticed an integration test was taking 0 to 5 seconds to delete a table.
Investigated this and found some code in delete table that was waiting
on the TabletGroupWatcher to cycle as the cause of the delay.
Investigated why this wait was there and could not find a definite
answer. Speculating that the purpose of the wait was to deal with
possible concurrent writes to tablet metadata by the tablet group
watcher in the case it has not yet noticed the table state is DELETING.
Changes in elasticity make this wait unnecessary because the delete table
fate sets an operation id on each tablet that should prevent any writes
to the tablet after its set. After being set this would prevent any
in flight writes by the tablet group watcher from succeeding. Before
elasticity there was no way for the delete table fate to prevent writes
by the tablet group watcher, so thinking it just waited until it was
certain the tablet group watcher was aware of the new table state and
would therefore do no writes.
While researching I noticed the delete table code in 2.1 logged per
tablet information about any tablet its was waiting on. This was not
being done in elasticity, so added logging for this.
With these changes TabletManagementIteratorIT went from 1 min runtime to
30 second runtime. The test deleted 9 tables.
---
.../java/org/apache/accumulo/manager/Manager.java | 10 ----------
.../apache/accumulo/manager/state/TableStats.java | 4 ----
.../accumulo/manager/tableOps/delete/CleanUp.java | 23 ----------------------
.../manager/tableOps/delete/DeleteTable.java | 2 +-
.../manager/tableOps/delete/ReserveTablets.java | 13 +++++++++++-
5 files changed, 13 insertions(+), 39 deletions(-)
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
index 935ca7d262..95fb00777e 100644
--- a/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
+++ b/server/manager/src/main/java/org/apache/accumulo/manager/Manager.java
@@ -520,16 +520,6 @@ public class Manager extends AbstractServer
}
}
- public boolean hasCycled(long time) {
- for (TabletGroupWatcher watcher : watchers) {
- if (watcher.stats.lastScanFinished() < time) {
- return false;
- }
- }
-
- return true;
- }
-
public void clearMigrations(TableId tableId) {
synchronized (migrations) {
migrations.keySet().removeIf(extent -> extent.tableId().equals(tableId));
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/state/TableStats.java
b/server/manager/src/main/java/org/apache/accumulo/manager/state/TableStats.java
index 3e2380a57c..c0d9a4f5e3 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/state/TableStats.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/state/TableStats.java
@@ -76,10 +76,6 @@ public class TableStats {
return endScan - startScan;
}
- public synchronized long lastScanFinished() {
- return endScan;
- }
-
@Override
public String toString() {
return new StringBuilder().append("last:
").append(last.toString()).toString();
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/CleanUp.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/CleanUp.java
index 9d8d2b86f6..9dd234eba9 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/CleanUp.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/CleanUp.java
@@ -61,32 +61,9 @@ class CleanUp extends ManagerRepo {
private long creationTime;
- private void readObject(java.io.ObjectInputStream in) throws IOException,
ClassNotFoundException {
- in.defaultReadObject();
-
- // handle the case where we start executing on a new machine where the
current time is in the
- // past relative to the previous machine
- // if the new machine has time in the future, that will work ok w/
hasCycled
- if (System.currentTimeMillis() < creationTime) {
- creationTime = System.currentTimeMillis();
- }
-
- }
-
public CleanUp(TableId tableId, NamespaceId namespaceId) {
this.tableId = tableId;
this.namespaceId = namespaceId;
- creationTime = System.currentTimeMillis();
- }
-
- @Override
- public long isReady(FateId fateId, Manager manager) throws Exception {
- // ELASTICITY_TODO investigate this, what is it for and is it still needed?
- if (!manager.hasCycled(creationTime)) {
- return 50;
- }
-
- return 0;
}
@Override
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
index 1ad900ca16..893fff7fe9 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
@@ -55,7 +55,7 @@ public class DeleteTable extends ManagerRepo {
final EnumSet<TableState> expectedCurrStates =
EnumSet.of(TableState.ONLINE, TableState.OFFLINE);
env.getTableManager().transitionTableState(tableId, TableState.DELETING,
expectedCurrStates);
- env.getEventCoordinator().event(tableId, "deleting table %s ", tableId);
+ env.getEventCoordinator().event(tableId, "deleting table %s %s", tableId,
fateId);
return new ReserveTablets(tableId, namespaceId);
}
diff --git
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/ReserveTablets.java
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/ReserveTablets.java
index 25c2b7a58f..f065206b2f 100644
---
a/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/ReserveTablets.java
+++
b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/ReserveTablets.java
@@ -81,13 +81,20 @@ public class ReserveTablets extends ManagerRepo {
tabletsSeen++;
if (tabletMeta.getLocation() != null) {
locations++;
+ log.debug("Delete table is waiting on tablet unload {} {} {}",
tabletMeta.getExtent(),
+ tabletMeta.getLocation(), fateId);
}
if (tabletMeta.getOperationId() != null) {
if (!opid.equals(tabletMeta.getOperationId())) {
otherOps++;
+ log.debug("Delete table is waiting on tablet with operation {} {}
{}",
+ tabletMeta.getExtent(), tabletMeta.getOperationId(), fateId);
}
} else {
+ // Its ok to set the operation id on a tablet with a location, but
after setting it we
+ // must wait for the tablet to have no location before proceeding to
actually delete. See
+ // the documentation about the opid column in the MetadataSchema
class for more details.
conditionalMutator.mutateTablet(tabletMeta.getExtent()).requireAbsentOperation()
.putOperation(opid).submit(tm ->
opid.equals(tm.getOperationId()));
submitted++;
@@ -96,11 +103,15 @@ public class ReserveTablets extends ManagerRepo {
}
if (locations > 0 || otherOps > 0 || submitted != accepted.get()) {
- log.debug("{} Waiting to delete table locations:{} operations:{}
submitted:{} accepted:{}",
+ log.info("{} Waiting to delete table locations:{} operations:{}
submitted:{} accepted:{}",
fateId, locations, otherOps, submitted, accepted.get());
return Math.min(Math.max(100, tabletsSeen), 30000);
}
+ // Once all tablets have the delete opid column set AND no tablets have a
location set then its
+ // safe to proceed with deleting the tablets. These two conditions being
true should prevent any
+ // concurrent writes to tablet metadata by other threads assuming they are
using conditional
+ // writes with standard conditional checks.
return 0;
}
