You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/06/15 19:22:46 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #2169: Cancel compactions when delete table is called
dlmarion opened a new pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169
Call cancelCompaction before deleting a table
Closes #2030
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#issuecomment-861790721
I'm going to add a test to ExternalCompactionIT to test deleting a table during a user compaction. We have a test where a table delete happens during a system compaction, but not during a user compaction
--
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656374533
##########
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)
Review comment:
We could make sure the lock ID used for the read is the same as that used by the delete write lock. I would expect that one holding the write lock could also get a read lock, since that's the normal behavior of ReadWriteLock. But you can't upgrade a read lock to a write lock.
--
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:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r657032042
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/cancel/CancelCompactions.java
##########
@@ -54,6 +54,19 @@ public long isReady(long tid, Manager env) throws Exception {
@Override
public Repo<Manager> call(long tid, Manager environment) throws Exception {
+
+ mutateZooKeeper(tid, tableId, environment);
+ return new FinishCancelCompaction(namespaceId, tableId);
+ }
+
+ @Override
+ public void undo(long tid, Manager env) {
+ Utils.unreserveTable(env, tableId, tid, false);
+ Utils.unreserveNamespace(env, namespaceId, tid, false);
+ }
Review comment:
I noticed that the `undo()` of CancelCompaction doesn't actually undo the mutate of ZooKeeper. From my understanding, the `undo()` of a FATE REPO is called if the operation fails. This may not be a problem for this REPO but I think there other operations that fail to actually undo anything from the operation and just call these 2 unreserve methods. I think this is something that we should probably look into.
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#issuecomment-1043114516
synchronizing this method shouldn't hurt
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656296006
##########
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)
Review comment:
Each FATE Repo has to be able to fail at any point and be re-run. As for the FATE lock I have no idea what the behavior would be, I have not looked at the code in a really long time. Would just need to examine the code to see what it might do. Also I do see some sort of unit test, maybe it could be used for experimentation.
--
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r655648420
##########
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.
--
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
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.
--
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r655722917
##########
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)
Review comment:
@dlmarion did you look into the behavior for the following situation :
1. gets read lock
2. cancels
3. release read lock
4. get write lock
5. manager process dies
6. manager process starts up again
7. isReady is called again and it attempts to get the read lock again
When step 7 runs and tries to get the read lock, the FATE tx will already be holding the write lock because of the failure. What will happen in this case?
--
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r655722917
##########
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)
Review comment:
@dlmarion did you look into the behavior for the following situation :
1. gets read lock
2. cancels
3. release read lock
4. get write lock
5. manager process dies
6. manager process starts up again
7. isReady is called again and it attempts to get the read lock again
When step 7 runs and tries to get the read lock, the FATE tx will already be holding the write lock because of the failure. What will happen in this case?
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656584622
##########
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)
Review comment:
I think I might have it. I'm going to open another PR here shortly
--
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r652169812
##########
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:
If were to change the code pass a tableId to the server instead of a tablename, this could cause thrift incompat problems.
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656567474
##########
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)
Review comment:
ok, this is going to take longer than I thought. I'm trying to understand how the locks work. Do you want me to revert the commit?
--
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:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r657153416
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/cancel/CancelCompactions.java
##########
@@ -54,6 +54,19 @@ public long isReady(long tid, Manager env) throws Exception {
@Override
public Repo<Manager> call(long tid, Manager environment) throws Exception {
+
+ mutateZooKeeper(tid, tableId, environment);
+ return new FinishCancelCompaction(namespaceId, tableId);
+ }
+
+ @Override
+ public void undo(long tid, Manager env) {
+ Utils.unreserveTable(env, tableId, tid, false);
+ Utils.unreserveNamespace(env, namespaceId, tid, false);
+ }
Review comment:
That is good point. Perhaps that is why nothing is done in the undo impl. I guess the undo doesn't always work for situations like this. This made me think of a recent undo failure I saw though while doing seom Random walk testing using the MutlitTable setup. https://github.com/apache/accumulo/issues/1919. It still think it would be good to revisit the FATE operations for things like this.
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r654364224
##########
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:
That's an interesting thought that I did not explore. I can look at that. I did look at having the Delete fate op call the cancelCompaction fate op, and that seemed overly complex.
##########
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:
@ctubbsii - I backed out my change to TableOperationsImpl and modified the DeleteTable fate transaction in the manner you described.
--
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:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#issuecomment-1043050205
@dlmarion @keith-turner I noticed a shared static method between 2 separate FATE operations that was added in this PR and was wondering if this could cause concurrency problems. https://github.com/apache/accumulo/blob/3b2cdb99aeb60dfcc58eba26b6620825aadb560b/server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/cancel/CancelCompactions.java#L67
I am not sure of what the situation would be but the fact that these two operations mutate the same node in ZK (compact-cancel-id) makes me curious. If they are both running and hit ZK problems, I wonder if a cancel compaction may cause a delete table to fail.
Delete Table (PreDeleteTable)
Cancel compaction (CancelCompactions)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r655652046
##########
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656581800
##########
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)
Review comment:
Reverting or opening an issue that is a blocker for 2.1 seem like two good ways to handle 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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r654123699
##########
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:
> If the cancelCompaction never returns and the delete never gets submitted I don't think there is any harm here.
I disagree with this strongly.
Specifically, I'm concerned about the impact on user experience for injecting another client-server RPC call and series of ZK operations in the pipeline prior to the user's intent to delete the table being recorded as a new FaTE. If anything happens in that pipeline that hangs, the user's intent to delete the table is never recorded... and that could be very frustrating.... like hitting the button on a crosswalk or elevator and nothing happening.
This could be especially frustrating because the user may be deleting as a consequence of prior table failures, and delete is being done as a last resort for recovery (or as the easiest resort, in the case of tables a user considers disposable). Because delete is so destructive, it is often used as a hammer when a table is in such a bad state as to need starting over. Having another operation precede it means that there are additional failure points that can prevent this hammer from being swung.
As for the earlier question:
> how many tables are being deleted concurrently?
I know of some users who create and delete tables at a pace most would consider extreme. I'm not sure how these changes would impact them.
##########
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:
Since all we're doing is using the read lock to update a ZK entry, I'm wondering if we can reduce the number of intermediate operations by performing this step early in the delete FaTE RepOs, rather than doing this client side. Can't the delete table RepO get the read lock first, update the ZK entry, then wait on the write lock?
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r657113415
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/cancel/CancelCompactions.java
##########
@@ -54,6 +54,19 @@ public long isReady(long tid, Manager env) throws Exception {
@Override
public Repo<Manager> call(long tid, Manager environment) throws Exception {
+
+ mutateZooKeeper(tid, tableId, environment);
+ return new FinishCancelCompaction(namespaceId, tableId);
+ }
+
+ @Override
+ public void undo(long tid, Manager env) {
+ Utils.unreserveTable(env, tableId, tid, false);
+ Utils.unreserveNamespace(env, namespaceId, tid, false);
+ }
Review comment:
I don't know that you can undo some of these operations. In this case, CancelCompaction mutates the value of a znode, which is being watched by processes performing compactions. Once it's mutated, and the processes notice, there is no going back. I'm not sure what would happen if you set the znode value back to its original value.
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656161829
##########
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)
Review comment:
I did not look into that behavior, I just implemented @ctubbsii suggestion about moving the logic server-side. I don't know enough about FATE to even know that issue would occur. Is there a way to test if this FATE transaction has a read or write lock held already?
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169
--
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r655652046
##########
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?
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656378302
##########
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)
Review comment:
I think I can easily add a method to check if the Fate transaction already has the write lock. I should have something later this afternoon.
--
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:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r652168564
##########
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:
To avoid certain race conditions, it would be nice if we did something like the following to only resolve the tableName to a tableId once. However I don't see an easy way to do this.
1. tableId = resolveTableId(tableName)
2. cancelCompaction(tableId)
3. delete(tableId)
Looking around in the code, this would be easy for cancelCompaction because it already resolves the table id client side. So could create `private cancelCompaction(TableId)` used by this method and the public `cancelCompaction(String)` method. However delete table seems like its currently passing the table name to the server side where it would resolve the table name to a table id server side. I may have missed it but I tried to follow the code to the thrift call and did not see anything on the client side resolving the table id for delete table.
--
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:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a change in pull request #2169: Cancel compactions when delete table is called
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #2169:
URL: https://github.com/apache/accumulo/pull/2169#discussion_r656590816
##########
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)
Review comment:
PR up at https://github.com/apache/accumulo/pull/2175
--
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:
users@infra.apache.org