You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/02/23 20:44:04 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3202: replace sleepUninterruptibly, improve interrupt handling
EdColeman opened a new pull request, #3202:
URL: https://github.com/apache/accumulo/pull/3202
Replaces sleepUninterruptibly with static method in a.a./core/util/UtilWaitThread that can be
interrupted, logs if an interrupt occurs and preserves the interrupt flag for handing higher in the call stack
- replace `sleepUninterruptibly()` with `UtilWaitThread.sleep(long duration, TimeUnit units)
- Improve interrupt handling:
- where InterruptException was already declared, throw an Interrupt exception
- where exception handling in place (or in tests) throw an IllegalStateException
- throw either AccumuloException or IOException when those were previously declared.
- In places with busy loop, test the interrupt flag to break out of the loop
- otherwise, maintain behaviour and ignore the interrupt.
- where the interrupt is not handled, tag with `// ignore interrupt status` to allow for searching
- Deprecate UtilWaitThread.sleep(long duration) with the new method that takes a TimeUnit
- Use TimeUnit.X with static imports for sleep, - changes some other TimeUnit usages same file.
- Minor checkstyle fixes.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117080765
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
There are places where testing the return value seemed more explicit than testing the flag (see CompactionCoordinator, line 219)
```
if (!UtilWaitThread.sleep(1000, MILLISECONDS)) {
throw new InterruptedException("Interrupted during pause trying to get coordinator lock");
}
```
In some places, this was used to throw an exiting exception (IOException, AccumuloException)
I avoided throwing the InterruptedException because there are so many places where we currently do nothing and that seemed like we would need to add a lot of empty try / catches. Not advocating that's the "best" way to handle things, but it is what we have now and this is trying to ease us into handling them, but that should be tacked over time.
--
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] dlmarion commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1116276439
##########
core/src/main/java/org/apache/accumulo/core/util/UtilWaitThread.java:
##########
@@ -18,17 +18,45 @@
*/
package org.apache.accumulo.core.util;
+import static java.util.concurrent.TimeUnit.MICROSECONDS;
+
+import java.util.concurrent.TimeUnit;
+
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class UtilWaitThread {
private static final Logger log = LoggerFactory.getLogger(UtilWaitThread.class);
+ /**
+ * Sleep for specified duration in milliseconds. If the sleep is interrupted, a message is logged
+ * and the interrupt flag is reset, but otherwise the interrupt is ignored.
+ * <p>
+ *
+ * @deprecated Use {@link UtilWaitThread#sleep(long, TimeUnit)} instead.
+ */
+ @Deprecated(since = "3.0.0")
public static void sleep(long millis) {
+ sleep(millis, MICROSECONDS);
Review Comment:
Should the time unit here be MILLISECONDS?
--
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] dlmarion commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117254686
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -704,11 +704,15 @@ private void invalidateSession(SessionID sessionId, HostAndPort location)
throw new TimedOutException(Collections.singleton(location.toString()));
}
- sleepUninterruptibly(sleepTime, MILLISECONDS);
+ // interrupt checked in while condition
+ UtilWaitThread.sleep(sleepTime, MILLISECONDS);
sleepTime = Math.min(2 * sleepTime, MAX_SLEEP);
}
-
+ if (Thread.currentThread().isInterrupted()) {
+ throw new TimedOutException(
+ "Interrupted during sleep: " + Collections.singleton(location.toString()));
+ }
Review Comment:
Why not just call Thread.sleep() and throw InterruptedException then? It has the same effect with less code.
--
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 pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#issuecomment-1454029119
> When that is truly intended we should make that explicit through the code and not as a side effect of the method.
The choice to use that method is "explicit through the code". The mere fact of using this method makes it appear that this is the true intention. It's not clear, without looking at each case individually, whether it's a side effect or the intent, but I would generally assume that it's intended if this method is used.
--
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] EdColeman commented on pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#issuecomment-1453852972
Replaced with https://github.com/apache/accumulo/pull/3225 that makes the replacement only in test code - leaving the other occurrences for future work.
--
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] cshannon commented on pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#issuecomment-1446518418
What is the motivation for making this change? This seems like a pretty big change in behavior going from sleeping even when interrupted vs no longer sleeping. I'm not saying it's necessarily wrong to switch it, I'm just curious the motivation and what kind of consequences (both intended and maybe unintended) that will occur as a result of 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117197375
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
> If we elect to continue to use Uninterruptibles I would like to hear the need that we must sleep for the entire time, ignoring the interrupt.
Typically when you call `Thread.sleep` and ignore the InterruptedException, you are waiting on some other condition to be met that is outside the control of the code. Many times, this is done inside of a loop, and you don't want to exit the loop, you just want to wait.
--
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] EdColeman commented on pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#issuecomment-1446538760
The use of sleepUninterruptibly will wait for the entire duration of the sleep, which seems to be a case that should be rare in our code to be the desired behavior. When that is truly intended we should make that explicit through the code and not as a side effect of the method.
In general, it felt like we are using sleepUninterruptibly excessively to avoid handling or re-throwing InterruptedException. I was trying to move the code into a direction where we can improve InterruptException handling rather than assuming that the can be ignored.
--
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] EdColeman closed pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman closed pull request #3202: replace sleepUninterruptibly, improve interrupt handling
URL: https://github.com/apache/accumulo/pull/3202
--
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] dlmarion commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117117907
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
> there are so many places where we currently do nothing and that seemed like we would need to add a lot of empty try / catches
That's why I was thinking that continuing to use Uninterruptibles in these places makes sense. Ignore where you really want to ignore, handle where you really want to handle. I'm still curious what errorprone has to say.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1116282663
##########
core/src/main/java/org/apache/accumulo/core/util/UtilWaitThread.java:
##########
@@ -18,17 +18,45 @@
*/
package org.apache.accumulo.core.util;
+import static java.util.concurrent.TimeUnit.MICROSECONDS;
+
+import java.util.concurrent.TimeUnit;
+
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class UtilWaitThread {
private static final Logger log = LoggerFactory.getLogger(UtilWaitThread.class);
+ /**
+ * Sleep for specified duration in milliseconds. If the sleep is interrupted, a message is logged
+ * and the interrupt flag is reset, but otherwise the interrupt is ignored.
+ * <p>
+ *
+ * @deprecated Use {@link UtilWaitThread#sleep(long, TimeUnit)} instead.
+ */
+ @Deprecated(since = "3.0.0")
public static void sleep(long millis) {
+ sleep(millis, MICROSECONDS);
Review Comment:
Resolved with 5f2c1cb00aed8d8
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117126418
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
Running error prone explicitly (at the current level) says nothing. I also was under the impression that we now run error prone by default, but I just did
```
mvn verify -Perrorprone -Dtest=blah -Dit.test=blah
```
to double check.
--
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] dlmarion commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1116937269
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -679,7 +679,7 @@ private void invalidateSession(SessionID sessionId, HostAndPort location)
LockID lid = new LockID(context.getZooKeeperRoot() + Constants.ZTSERVERS, sessionId.lockId);
- while (true) {
+ while (true && !Thread.currentThread().isInterrupted()) {
Review Comment:
I think that prior to this change this loop would exit either when invalidation was successful or if an error was thrown. I think this introduces a new condition where the loop exits but it's not successful nor is an error thrown. I'm assuming here (because I didn't look) that the calling code will assume success when this returns, even though it may not have succeeded. This would be a good place to throw InterruptedException if we want to handle it.
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
There are a lot of places in this PR where we are using this new method but intentionally ignoring the return value and the thread interrupt status. For example:
```
// ignore interrupt status
UtilWaitThread.sleep(100, MILLISECONDS);
```
I think this may be incorrect. If we are really going to ignore the thread interrupt status, then we should be resetting it back to `false` if it is `true`. I think, in the cases like this where we are ignoring the interrupt and return value, calling `Uninterruptibles.sleepUninterruptibly` is the right thing to do.
--
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] EdColeman commented on pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#issuecomment-1454047105
The use in tests seems to contradict the assumption that the intent was to leverage the side effect of pausing the full duration - but rather it looks like it was used as a convenience to avoid handling or explicitly throwing InterruptedException.
Because of the number of places that sleepUninterrupty is used made this PR very large and hard to review. I did not expect the push-back, but do understand the concern. Basically I think the default should be to bail when an interrupt occurs unless in very specific cases where is can be handled.
In the future, we should be on the look out for new uses of sleepUninterrupty and at a minimum require documentation that the side effect is necessary for correct operation.
There are other places with busy loop that seem like they would benefit from explicitly recognizing the interrupt status and those can be handled on a case by case basis.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117034913
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
Over time, we should look to improving the interrupt handling and this is a step in getting there. Other than allowing an earlier return on interrupt, this preserves the current behavior.
The only difference between using `UtilWaitThread.sleep` and ignoring the interrupt as done here and using `sleepUninterruptibly` is that we will now exit the sleep on an interrupt (and log a message). sleepUninterruptibly catches the interrupt and then goes back to sleep for the remaining time. If an interrupt occurred, sleepUninterruptibly also sets the interrupted flag on return if an interrupt occurred, but we currently never look at 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117062523
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
> sleepUninterruptibly also sets the interrupted flag on return if an interrupt occurred
ok, I was lazy and didn't look. Thanks. Have you built this with `errorprone`? I'm wondering if it complains about the return value being unchecked. Is there a reason to return a `boolean` if the pattern is for the user to check if the Thread was interrupted after calling this method?
I would think that it would be more explicit to throw the InterruptedException when it's thrown in the current thread (e.g. Thread.sleep() ) and check the interrupt status on another thread.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117157344
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
If we elect to continue to use Uninterruptibles I would like to hear the need that we *must* sleep for the entire time, ignoring the interrupt.
Running error prone at the next effort level, I do not see anything - it may be seeing that the interrupt flag is set as a side effect - but we fail pretty fast in core because of other (known) issues, so not really a strong test.
If it does get flagged, I would advocate that we create a similar method `UtilWaitThread.sleepQuitely` or something so that we still break out of the sleep on interrupts.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117080765
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
There are places where testing the return value seemed more explicit than testing the flag (see CompactionCoordinator, line 219)
```
if (!UtilWaitThread.sleep(1000, MILLISECONDS)) {
throw new InterruptedException("Interrupted during pause trying to get coordinator lock");
}
```
In some places, this was used to throw an exiting exception (IOException, AccumuloException)
I avoided throwing the InterruptedException because there are so many places where we currently do nothing and that seemed like we would need to add a lot of empty try / catches. Not advocating that's the "best" way to handle things, but it is what we have now and this is trying to ease us into handling them, but that should be tackled over time.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117121719
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -679,7 +679,7 @@ private void invalidateSession(SessionID sessionId, HostAndPort location)
LockID lid = new LockID(context.getZooKeeperRoot() + Constants.ZTSERVERS, sessionId.lockId);
- while (true) {
+ while (true && !Thread.currentThread().isInterrupted()) {
Review Comment:
See 60f14d86ef8b - the other option would be to remove the interrupt flag check and ignore as done before.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117157344
##########
core/src/main/java/org/apache/accumulo/core/client/IsolatedScanner.java:
##########
@@ -114,7 +114,8 @@ private void readRow() {
}
// wait a moment before retrying
- sleepUninterruptibly(100, MILLISECONDS);
+ // ignore interrupt status
+ UtilWaitThread.sleep(100, MILLISECONDS);
Review Comment:
If we elect to continue to use Uninterruptibles I would like to hear the need that we *must* sleep for the entire time, ignoring the interrupt.
Running error prone at the next effort level, I do not see anything - is may be seeing that the interrupt flag is set as a side effect - but we fail pretty fast in core because of other (known) issues, so not really a strong test.
If it does get flagged, I would advocate that we create a similar method `UtilWaitThread.sleepQuitely` or something so that we still break out of the sleep on interrupts.
--
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] EdColeman commented on a diff in pull request #3202: replace sleepUninterruptibly, improve interrupt handling
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on code in PR #3202:
URL: https://github.com/apache/accumulo/pull/3202#discussion_r1117258703
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -704,11 +704,15 @@ private void invalidateSession(SessionID sessionId, HostAndPort location)
throw new TimedOutException(Collections.singleton(location.toString()));
}
- sleepUninterruptibly(sleepTime, MILLISECONDS);
+ // interrupt checked in while condition
+ UtilWaitThread.sleep(sleepTime, MILLISECONDS);
sleepTime = Math.min(2 * sleepTime, MAX_SLEEP);
}
-
+ if (Thread.currentThread().isInterrupted()) {
+ throw new TimedOutException(
+ "Interrupted during sleep: " + Collections.singleton(location.toString()));
+ }
Review Comment:
This logs and resets the interrupt flag. Throwing an existing exception delegates to exception handling in place. Throwing InterruptException would require more changes.
--
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