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/23 15:23:06 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #2178: Catch NoNodeException in CompactRange
milleruntime opened a new pull request #2178:
URL: https://github.com/apache/accumulo/pull/2178
* Fixes #1919
* It is possible for a compaction to run after a table is deleted so
catch the exception and print to debug, avoiding the FATE warning
--
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 #2178: Catch NoNodeException in CompactRange
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2178:
URL: https://github.com/apache/accumulo/pull/2178#discussion_r657327267
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactRange.java
##########
@@ -149,24 +149,27 @@ static void removeIterators(Manager environment, final long txid, TableId tableI
ZooReaderWriter zoo = environment.getContext().getZooReaderWriter();
- zoo.mutateExisting(zTablePath, currentValue -> {
- String cvs = new String(currentValue, UTF_8);
- String[] tokens = cvs.split(",");
- long flushID = Long.parseLong(tokens[0]);
-
- String txidString = String.format("%016x", txid);
+ try {
+ zoo.mutateExisting(zTablePath, currentValue -> {
+ String cvs = new String(currentValue, UTF_8);
+ String[] tokens = cvs.split(",");
+ long flushID = Long.parseLong(tokens[0]);
- StringBuilder encodedIterators = new StringBuilder();
- for (int i = 1; i < tokens.length; i++) {
- if (tokens[i].startsWith(txidString))
- continue;
- encodedIterators.append(",");
- encodedIterators.append(tokens[i]);
- }
+ String txidString = String.format("%016x", txid);
- return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
- });
+ StringBuilder encodedIterators = new StringBuilder();
+ for (int i = 1; i < tokens.length; i++) {
+ if (tokens[i].startsWith(txidString))
+ continue;
+ encodedIterators.append(",");
+ encodedIterators.append(tokens[i]);
+ }
+ return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
+ });
+ } catch (NoNodeException ke) {
+ log.debug("Node for {} no longer exists.", tableId, ke);
+ }
Review comment:
I'm not sure how it's possible for this to run after a table is deleted, unless the lock reservations are relying on ZooCache to check table existence and it's somehow out-of-date. See a related comment about compaction IDs in https://github.com/apache/accumulo/pull/2175#discussion_r657292064
In any case, elsewhere in that same class, we `throw new AcceptableThriftTableOperationException`, in response to `NoNodeException`. Should we do that here as well? I'm not sure who is even catching and handling that exception type. But, it's probably worth tracking down to see if they should be made consistent, or at least adding a comment what's different here that warrants a different reaction to the same exception.
--
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 #2178: Catch NoNodeException in CompactRange
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2178:
URL: https://github.com/apache/accumulo/pull/2178#discussion_r657366001
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactRange.java
##########
@@ -149,24 +149,27 @@ static void removeIterators(Manager environment, final long txid, TableId tableI
ZooReaderWriter zoo = environment.getContext().getZooReaderWriter();
- zoo.mutateExisting(zTablePath, currentValue -> {
- String cvs = new String(currentValue, UTF_8);
- String[] tokens = cvs.split(",");
- long flushID = Long.parseLong(tokens[0]);
-
- String txidString = String.format("%016x", txid);
+ try {
+ zoo.mutateExisting(zTablePath, currentValue -> {
+ String cvs = new String(currentValue, UTF_8);
+ String[] tokens = cvs.split(",");
+ long flushID = Long.parseLong(tokens[0]);
- StringBuilder encodedIterators = new StringBuilder();
- for (int i = 1; i < tokens.length; i++) {
- if (tokens[i].startsWith(txidString))
- continue;
- encodedIterators.append(",");
- encodedIterators.append(tokens[i]);
- }
+ String txidString = String.format("%016x", txid);
- return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
- });
+ StringBuilder encodedIterators = new StringBuilder();
+ for (int i = 1; i < tokens.length; i++) {
+ if (tokens[i].startsWith(txidString))
+ continue;
+ encodedIterators.append(",");
+ encodedIterators.append(tokens[i]);
+ }
+ return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
+ });
+ } catch (NoNodeException ke) {
+ log.debug("Node for {} no longer exists.", tableId, ke);
+ }
Review comment:
I think it is just a timing thing. If you look at the stacktrace in #1919 you can see it was waiting for the table read lock. It is odd that `CompactRange` doesn't need a write lock but DeleteTable does.
--
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 #2178: Catch NoNodeException in CompactRange
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2178:
URL: https://github.com/apache/accumulo/pull/2178#discussion_r657562458
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactRange.java
##########
@@ -149,24 +149,27 @@ static void removeIterators(Manager environment, final long txid, TableId tableI
ZooReaderWriter zoo = environment.getContext().getZooReaderWriter();
- zoo.mutateExisting(zTablePath, currentValue -> {
- String cvs = new String(currentValue, UTF_8);
- String[] tokens = cvs.split(",");
- long flushID = Long.parseLong(tokens[0]);
-
- String txidString = String.format("%016x", txid);
+ try {
+ zoo.mutateExisting(zTablePath, currentValue -> {
+ String cvs = new String(currentValue, UTF_8);
+ String[] tokens = cvs.split(",");
+ long flushID = Long.parseLong(tokens[0]);
- StringBuilder encodedIterators = new StringBuilder();
- for (int i = 1; i < tokens.length; i++) {
- if (tokens[i].startsWith(txidString))
- continue;
- encodedIterators.append(",");
- encodedIterators.append(tokens[i]);
- }
+ String txidString = String.format("%016x", txid);
- return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
- });
+ StringBuilder encodedIterators = new StringBuilder();
+ for (int i = 1; i < tokens.length; i++) {
+ if (tokens[i].startsWith(txidString))
+ continue;
+ encodedIterators.append(",");
+ encodedIterators.append(tokens[i]);
+ }
+ return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
+ });
+ } catch (NoNodeException ke) {
+ log.debug("Node for {} no longer exists.", tableId, ke);
+ }
Review comment:
I checked where we `throw new AcceptableThriftTableOperationException`, and it seems we mostly do it in `isReady` and `call`, but in `CompactRange`, we do it in the constructor. There are no other places where we do it anywhere else (except in helper methods that get called by one of these). There are no places where we throw it in `undo`, so I'm fine with the code as you have 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] milleruntime merged pull request #2178: Catch NoNodeException in CompactRange
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #2178:
URL: https://github.com/apache/accumulo/pull/2178
--
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 #2178: Catch NoNodeException in CompactRange
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #2178:
URL: https://github.com/apache/accumulo/pull/2178#discussion_r657382125
##########
File path: server/manager/src/main/java/org/apache/accumulo/manager/tableOps/compact/CompactRange.java
##########
@@ -149,24 +149,27 @@ static void removeIterators(Manager environment, final long txid, TableId tableI
ZooReaderWriter zoo = environment.getContext().getZooReaderWriter();
- zoo.mutateExisting(zTablePath, currentValue -> {
- String cvs = new String(currentValue, UTF_8);
- String[] tokens = cvs.split(",");
- long flushID = Long.parseLong(tokens[0]);
-
- String txidString = String.format("%016x", txid);
+ try {
+ zoo.mutateExisting(zTablePath, currentValue -> {
+ String cvs = new String(currentValue, UTF_8);
+ String[] tokens = cvs.split(",");
+ long flushID = Long.parseLong(tokens[0]);
- StringBuilder encodedIterators = new StringBuilder();
- for (int i = 1; i < tokens.length; i++) {
- if (tokens[i].startsWith(txidString))
- continue;
- encodedIterators.append(",");
- encodedIterators.append(tokens[i]);
- }
+ String txidString = String.format("%016x", txid);
- return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
- });
+ StringBuilder encodedIterators = new StringBuilder();
+ for (int i = 1; i < tokens.length; i++) {
+ if (tokens[i].startsWith(txidString))
+ continue;
+ encodedIterators.append(",");
+ encodedIterators.append(tokens[i]);
+ }
+ return (Long.toString(flushID) + encodedIterators).getBytes(UTF_8);
+ });
+ } catch (NoNodeException ke) {
+ log.debug("Node for {} no longer exists.", tableId, ke);
+ }
Review comment:
> In any case, elsewhere in that same class, we `throw new AcceptableThriftTableOperationException`, in response to `NoNodeException`. Should we do that here as well?
I don't think that is necessary. The `undo()` will only be called when the operation failed, which usually is when an Exception occurs. Throwing another exception on a failure in the `undo()` seems excessive. I think this is where it prints the original Exception which set the REPO to `FAILED_IN_PROGRESS`. https://github.com/apache/accumulo/blob/16a3cec36981a694ecdfbefb839ecf9ddd70a535/core/src/main/java/org/apache/accumulo/fate/Fate.java#L162-L174
I assume in this case the exception was acceptable so wasn't printed.
--
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