ctubbsii commented on a change in pull request #2175:
URL: https://github.com/apache/accumulo/pull/2175#discussion_r657292064
##########
File path:
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
##########
@@ -43,8 +43,13 @@ public DeleteTable(NamespaceId namespaceId, TableId tableId)
{
@Override
public long isReady(long tid, Manager env) throws Exception {
- // Before attempting to delete the table, cancel any running user
- // compactions.
+ // If we have the table write lock, then it's possible that we have
+ // already canceled the compactions and we can skip to deleting the tables.
+ if (Utils.hasNamespaceReadLock(env, namespaceId, tid)
+ && Utils.hasTableWriteLock(env, tableId, tid)) {
+ return 0;
+ }
+ // Cancel any running user compactions before deleting table
if (Utils.reserveNamespace(env, namespaceId, tid, false, true,
TableOperation.COMPACT_CANCEL)
Review comment:
I checked all our code for any references to `.call()` and the only time
`Repo.call()` is seen is in `Fate.java` after `.isReady()`. The only other
occurrence was in TraceRepo, whose `isReady` and `call` methods proxy the
wrapped Repo. So, `.call()` is never run without first running `.isReady()`.
So, I think 1-pre-step solution is fine (and preferred, because it's
simpler).
The only risk is running `mutateZooKeeper` multiple times, and I don't think
that's a big deal. First, it could also happen if there was a failure in the
2-step solution so it doesn't change anything get rid of the second step.
Second, we really want to make sure compactors get the message. The only other
concern I can think of is whether `mutateZooKeeper` is idempotent. It should
be, since that was already existing code in the CancelCompactions Repo.
There is one other thing I noticed about reusing the
`CancelCompactions.mutateZooKeeper` code, that we may wish to revisit: that
code seems to cancel currently running/scheduled compactions relative to some
`flushID`. However, for delete, we really want to block all possible future
compactions as well, so the implementation needed to cancel compactions for
DeleteTable can just set the compaction ID to a fixed maximum value, rather
than even bother checking what the current value is and mutating it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
