ctubbsii commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r655646862
##########
File path:
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
##########
@@ -41,6 +42,19 @@ 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 (Utils.reserveNamespace(env, namespaceId, tid, false, true,
TableOperation.COMPACT_CANCEL)
+ + Utils.reserveTable(env, tableId, tid, false, true,
TableOperation.COMPACT_CANCEL) == 0) {
+ try {
+ CancelCompactions.mutateZooKeeper(tid, tableId, env);
+ } finally {
+ Utils.unreserveTable(env, tableId, tid, false);
+ Utils.unreserveNamespace(env, namespaceId, tid, false);
Review comment:
It's too bad we can't hold the read lock and only release it after we
get the write lock, blocking the write lock from being taken by anybody else.
Delete should have priority.
##########
File path:
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -728,6 +728,7 @@ public void delete(String tableName)
List<ByteBuffer> args =
Arrays.asList(ByteBuffer.wrap(tableName.getBytes(UTF_8)));
Map<String,String> opts = new HashMap<>();
try {
+ cancelCompaction(tableName);
Review comment:
Cool. Thanks for indulging my concerns. Although they were probably not
too important, I do think your current implementation is better.
##########
File path:
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
##########
@@ -41,6 +42,19 @@ 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 (Utils.reserveNamespace(env, namespaceId, tid, false, true,
TableOperation.COMPACT_CANCEL)
+ + Utils.reserveTable(env, tableId, tid, false, true,
TableOperation.COMPACT_CANCEL) == 0) {
+ try {
+ CancelCompactions.mutateZooKeeper(tid, tableId, env);
+ } finally {
+ Utils.unreserveTable(env, tableId, tid, false);
+ Utils.unreserveNamespace(env, namespaceId, tid, false);
+ }
+ }
+
return Utils.reserveNamespace(env, namespaceId, tid, false, false,
TableOperation.DELETE)
+ Utils.reserveTable(env, tableId, tid, true, true,
TableOperation.DELETE);
Review comment:
Not related to your changes, but is it weird that reserveNamespace is
okay with the namespace not existing, but reserveTable requires the table to
exist? How can the table exist without its namespace? And, would we want to try
to delete a table that's in a non-existent namespace?
##########
File path:
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/delete/DeleteTable.java
##########
@@ -41,6 +42,19 @@ 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 (Utils.reserveNamespace(env, namespaceId, tid, false, true,
TableOperation.COMPACT_CANCEL)
+ + Utils.reserveTable(env, tableId, tid, false, true,
TableOperation.COMPACT_CANCEL) == 0) {
+ try {
+ CancelCompactions.mutateZooKeeper(tid, tableId, env);
+ } finally {
+ Utils.unreserveTable(env, tableId, tid, false);
+ Utils.unreserveNamespace(env, namespaceId, tid, false);
+ }
+ }
+
return Utils.reserveNamespace(env, namespaceId, tid, false, false,
TableOperation.DELETE)
+ Utils.reserveTable(env, tableId, tid, true, true,
TableOperation.DELETE);
Review comment:
Not related to your changes, but is it weird that reserveNamespace is
okay with the namespace not existing (mustExist flag is false), but
reserveTable requires the table to exist (mustExist flag is true)? How can the
table exist without its namespace? And, would we want to try to delete a table
that's in a non-existent namespace?
--
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]
