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 2022/04/11 18:10:02 UTC
[GitHub] [accumulo] nikita-sirohi opened a new pull request, #2622: Handle thread interrupt in server client execute loop.
nikita-sirohi opened a new pull request, #2622:
URL: https://github.com/apache/accumulo/pull/2622
Handling mirrors ThriftServer while loop handling.
Issue found while running trino + accumulo.
Resolves: #2621
--
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] nikita-sirohi commented on pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1102038583
👍 Sounds good. Thanks!
Will update the PR when I get a minute.
--
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] nikita-sirohi commented on pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1101042541
Thanks everyone for the feedback. Want to make sure I understand the salient points here:
* If we go with the "Thread.currentThread().isInterrupted()" solution, we should reset the interrupted state -- this makes 100% sense to me, can/will fix.
* Need to move the exception to the try/catch so that the finally executes (and add a bit more detail to the exception) -- again, makes total sense if we go with the while loop soln
* This patch addresses only one case, but there could be other places where this might happen. Makes sense. Wanted to make sure I understand a few points though @dlmarion :
> I think that ThriftUtil.createClientTransport should _not_ handle the InterruptedException and should let it be handled by the caller.
That was my inclination as well.
> In the ServerClient and ManagerClient cases, the InterruptedException should be handled outside of the while loop and Thread.interrupted should be called to reset the Threads' interrupt state.
Re-calling Thread.interrupted makes sense to me, but why handle this outside of the while loop as opposed to in the try/catch? Is the thought here surrounding the loops in a try/catch that checks for an InterruptedException and converts it to an AccumuloException? Or at that point, why check for the exception at all?
> For the TTimeoutTransport.create case, I wonder if the right solution is to catch ClosedByInterruptException in TTimeoutTransport.openSocket and throw a new InterruptedException.
This seems like a good option; my only concern here would be that the original exception is explicitly thrown as an IOException and the InterruptedException is _not_ thrown. We're essentially patching over that behavior. I don't understand well enough the reasoning behind the original behavior to know if this is a bad idea for some reason.
> For the TSaslClientTransport case, I don't think we have an opportunity to catch ClosedByInterruptException. Instead we would have to see if that might be the cause of one of the exceptions that it raises and then throw a new InterruptedException.
That makes sense -- could just check for (and then reset) thread state interrupted the way I did in the original PR.
--
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 pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1109676319
I'm good with these changes, they fix the original problem. I'm working in the Thrift code in another branch and can consolidate into a utility method if needed.
--
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] nikita-sirohi commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858029540
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -100,6 +103,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
return new TIOStreamTransport(input, output);
} catch (IOException e) {
closeSocket(socket, e);
Review Comment:
Uh... that snippet does not look nice... not sure how to format that nicely: but it's in TTimeoutTransport.java
--
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] nikita-sirohi commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858027449
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -100,6 +103,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
return new TIOStreamTransport(input, output);
} catch (IOException e) {
closeSocket(socket, e);
Review Comment:
Yes, but it'll then be caught and logged, not re thrown. Like so:
```java
private void closeSocket(Socket socket, Exception e) {
try {
if (socket != null)
socket.close();
} catch (IOException ioe) {
log.error("Failed to close socket after unsuccessful I/O stream setup", e);
}
}
```
--
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] nikita-sirohi commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858027449
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -100,6 +103,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
return new TIOStreamTransport(input, output);
} catch (IOException e) {
closeSocket(socket, e);
Review Comment:
Yes, but it'll then be caught and logged, not re thrown. Like so:
` private void closeSocket(Socket socket, Exception e) {
try {
if (socket != null)
socket.close();
} catch (IOException ioe) {
log.error("Failed to close socket after unsuccessful I/O stream setup", e);
}
}`
--
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] nikita-sirohi commented on pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1097029664
Re "What circumstance with that caused you to see this?":
Trino is a distributed SQL query engine: https://trino.io/ . Essentially, trino will spawn a number of threads to execute tasks against whatever the backing database/APIs are. If a query is aborted (ctrl+c as you asked about, etc), or a separate task fails, etc, the thread will be interrupted.
Most of the time I've seen this has been from ctrl+c called, but have seen this from queries failing/getting aborted in other ways as well (OOMs, or bad data).
Trino has some out of the box plugins, including one for accumulo, which from looking at the code I _suspect_ will have this same problem, but have not tried it out. We (my employer) wrote an us-specific plugin that uses trino to talk to accumulo/do some other things; we found that over time, performance would degrade until eventually no queries were serviceable. We saw the endless loop of "WARN ThriftUtil: Failed to open transport..." and traced from there.
Not at all surprised this is such an edge case -- was surprised to see that opening sockets while threads are interrupted works this way.
-------
Re "wasn't sure if either createClientTransport or TTimeoutTransport.create had the appropriate information to special case this exception" --
it looked to me like these functions were doing very specific things, and not doing much "logic" with exceptions beyond wrapping them as TTransportExceptions and handing them to the caller. I also wasn't sure if every caller would want to see this exception handled the same way. I also wasn't sure if there were other such exceptions that would need special casing.
------
Re "I'll have to see where this pattern is being used elsewhere... it feels wrong, but if it's being done that elsewhere, I'm curious why."
Honestly, beyond this and a bit of other debugging, I haven't spent a ton of time digging through accumulo internals. I saw the patch tracked in ACCUMULO-3030/77226b05bbfc204ec45e3aefb5ea580fb5b9b5af and did a best effort pattern match; not much more to it :) .
Let me know and will adjust the PR as needed. Thanks for all the conversation!
--
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] nikita-sirohi commented on pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1110294874
Will do, thanks!
--
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 merged pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
ctubbsii merged PR #2622:
URL: https://github.com/apache/accumulo/pull/2622
--
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] nikita-sirohi commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858039032
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -527,6 +529,12 @@ public static ServerAddress createSaslThreadPoolServer(HostAndPort address, TPro
serverUser = UserGroupInformation.getLoginUser();
} catch (IOException e) {
transport.close();
Review Comment:
It's enclosed in a try, and said exception would just be logged.
--
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] milleruntime commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r851222759
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ServerClient.java:
##########
@@ -88,6 +88,9 @@ public static <T> T executeRaw(ClientContext context,
public static <CT extends TServiceClient,RT> RT executeRaw(ClientContext context,
TServiceClientFactory<CT> factory, ClientExecReturn<RT,CT> exec) throws Exception {
while (true) {
+ if (Thread.currentThread().isInterrupted()) {
+ throw new AccumuloException("Thread interrupted");
Review Comment:
You could improve the `AccumuloException` by including more information and moving it down into the try/catch. You could include the `server` or there is a bunch of stuff in `ClientContext`.
--
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 pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1101339181
> Re-calling Thread.interrupted makes sense to me, but why handle this outside of the while loop as opposed to in the try/catch? Is the thought here surrounding the loops in a try/catch that checks for an InterruptedException and converts it to an AccumuloException? Or at that point, why check for the exception at all?
I guess I was looking at how TTransportException was handled (logged) and it didn't break out of the loop. As long as you break out of the loop somehow I think that is sufficient.
> This seems like a good option; my only concern here would be that the original exception is explicitly thrown as an IOException and the InterruptedException is not thrown.
It seems to me that `ClosedByInterruptException` is both an IOException and an InterruptedException as the reason it's thrown is the IO thread is interrupted by another thread, and then the interrupted state is set on the IO thread. I suggested throwing an InterruptedException in this case so that callers would know to handle the interrupted state appropriately.
> could just check for (and then reset) thread state interrupted the way I did in the original PR
You could do that, but there may be a case where the parent class to `ClosedByInterruptException` is thrown ( I don't actually know if this is true). `AsynchronousCloseException` is the parent, has a similar description, but does not describe setting the interrupt state.
I went back and read what @ctubbsii wrote:
> Another alternative to consider: everywhere we throw new TTransportException because of an IOException, we could check the IOException is an instanceof ClosedByInterruptException and instead of wrapping it with TTransportException, we throw UncheckedIOException and throw that instead. There's only a few such occurrences in our code.
If we changed it to the following, then I think we would handle more cases but be less intrusive:
Another alternative to consider: everywhere we throw new TTransportException in `ThriftUtil.createClientTransport` because of an IOException, we could:
1. check the IOException is an instanceof `AsynchronousCloseException`
2. check if the Thread is interrupted and if so reset the interrupt state
3. and instead of wrapping it with TTransportException, we throw UncheckedIOException instead.
--
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 diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r857898699
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -87,6 +84,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
socket = openSocket(addr, (int) timeoutMillis);
} catch (IOException e) {
// openSocket handles closing the Socket on error
+ if (e instanceof AsynchronousCloseException) {
+ if (Thread.currentThread().isInterrupted()) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedIOException(e);
+ }
+ }
Review Comment:
I think this strategy is okay, but I don't think we actually want to do this for other AsynchronousCloseExceptions. I'm not sure what exactly can cause those, but I imagine if those happen, they would happen because of very special edge cases we'd probably want to explicitly handle.
For this, I think it's sufficient to just check ClosedByInterruptException. The above code sets the interrupted state flag before it throws UncheckedIOException. I don't think that's necessary, since the flag will have already been set. Even if it happens that it had been cleared, I don't think it matters, because the intent is to let the UncheckedIOException propagate all the way up and terminate the thread. It doesn't really matter if the flag is true or false when the thread terminates this way.
So, more succinctly, I think these blocks can be written as:
```suggestion
if (e instanceof ClosedByInterruptException) {
throw new UncheckedIOException(e);
}
```
--
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 #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r857903733
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -87,6 +84,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
socket = openSocket(addr, (int) timeoutMillis);
} catch (IOException e) {
// openSocket handles closing the Socket on error
+ if (e instanceof AsynchronousCloseException) {
+ if (Thread.currentThread().isInterrupted()) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedIOException(e);
+ }
+ }
Review Comment:
@ctubbsii and I talked about this issue after I put up my most recent comment. Apologies for not updating the PR. He suggested a minor tweak to make the fix more targeted to the original issue.
```suggestion
if (e instanceof ClosedByInterruptException) {
Thread.currentThread().interrupt();
throw new UncheckedIOException(e);
}
```
--
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] nikita-sirohi commented on pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1109236386
Updated with requested changes. Thanks for the feedback.
> The other changes are suggestions, but I'm not 100% sure that's the solution we want to go with. I'm not super confident with handling this situation. @dlmarion WDYT?
Would it be best for us to jump on a quick call? Let me know.
--
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 diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858286888
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -100,6 +103,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
return new TIOStreamTransport(input, output);
} catch (IOException e) {
closeSocket(socket, e);
Review Comment:
@nikita-sirohi I formatted your code snippet. See https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/creating-and-highlighting-code-blocks#syntax-highlighting
--
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] nikita-sirohi commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858026071
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -87,6 +84,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
socket = openSocket(addr, (int) timeoutMillis);
} catch (IOException e) {
// openSocket handles closing the Socket on error
+ if (e instanceof AsynchronousCloseException) {
+ if (Thread.currentThread().isInterrupted()) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedIOException(e);
+ }
+ }
Review Comment:
Ok, makes sense. Will update
--
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] nikita-sirohi commented on pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1095817099
Thanks for the feedback & speedy response. You are correct -- normally we would see an InterruptedException get thrown.
In this case, though, what is actually thrown is an IOException in the form of ClosedByInterruptException when trying to open a socket: https://docs.oracle.com/javase/9/docs/api/java/nio/channels/ClosedByInterruptException.html .
"Checked exception received by a thread when another thread interrupts it while it is blocked in an I/O operation upon a channel. Before this exception is thrown the channel will have been closed and the interrupt status of the previously-blocked thread will have been set."
This ends up being swallowed and logged, so we see an endless loop of "WARN ThriftUtil: Failed to open transport..."
If you look in ThriftUtil.java, createClientTransport:
I see this code:
` try {
transport = TTimeoutTransport.create(address, timeout);
} catch (TTransportException e) {
log.warn("Failed to open transport to {}", address);
throw e;
}`
TTimeoutTransport.create ends up here:
` TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransportException {
Socket socket = null;
try {
socket = openSocket(addr, (int) timeoutMillis);
} catch (IOException e) {
// openSocket handles closing the Socket on error
throw new TTransportException(e);
}`
Since this manifests as an IOException, it's turned into a TTransportException. Gets logged and re thrown, then in the execute loop, retried forever.
I made the patch handle in this manner because this was the pattern I saw elsewhere -- in the ThriftServer while loop. So I thought this was the appropriate pattern. I also wasn't sure if either createClientTransport or TTimeoutTransport.create had the appropriate information to special case this exception -- felt like the caller should be handling it. More than happy to move this / handle differently though? Let me know.
--
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] nikita-sirohi commented on a diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
nikita-sirohi commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r858026247
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -18,14 +18,11 @@
*/
package org.apache.accumulo.core.rpc;
-import java.io.BufferedInputStream;
-import java.io.BufferedOutputStream;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStream;
+import java.io.*;
Review Comment:
Ah, my editor must have changed this. Will fix
--
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 diff in pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r857900245
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -527,6 +529,12 @@ public static ServerAddress createSaslThreadPoolServer(HostAndPort address, TPro
serverUser = UserGroupInformation.getLoginUser();
} catch (IOException e) {
transport.close();
Review Comment:
Can this close trigger its own IOException?
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -100,6 +103,12 @@ TTransport createInternal(SocketAddress addr, long timeoutMillis) throws TTransp
return new TIOStreamTransport(input, output);
} catch (IOException e) {
closeSocket(socket, e);
Review Comment:
Can this closeSocket trigger its own IOException?
##########
core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java:
##########
@@ -18,14 +18,11 @@
*/
package org.apache.accumulo.core.rpc;
-import java.io.BufferedInputStream;
-import java.io.BufferedOutputStream;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStream;
+import java.io.*;
Review Comment:
Our style does not permit use of wildcard imports.
Please use individual imports.
--
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 pull request #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
dlmarion commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1100081640
I'm thinking that this patch addresses only one case, but there could be other places where this might happen. For example, we have the same problem in `ManagerClient.execute`, `ManagerClient.executeGeneric`, and `ManagerClient.cancelFateOperation`. There are multiple places in ThriftUtil where the log message `Failed to open transport to {}` is logged. Two of those places are when calling `TTimeoutTransport.create` and the other two are when setting up `TSaslClientTransport`.
For the `TTimeoutTransport.create` case, I wonder if the right solution is to catch `ClosedByInterruptException` and throw a new `InterruptedException`. The Threads' `interrupted` state is already set when the `ClosedByInterruptException` is raised by the underlying Socket code. I think that `ThriftUtil.createClientTransport` should *not* handle the `InterruptedException` and should let it be handled by the caller. In the `ServerClient` and `ManagerClient` cases, the `InterruptedException` should be handled outside of the `while` loop and `Thread.interrupted` should be called to reset the Threads' interrupt state.
For the `TSaslClientTransport` case, I don't think we have an opportunity to catch `ClosedByInterruptException`. Instead we would have to see if that might be the cause of one of the exceptions that it raises and then throw a new InterruptedException.
--
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 #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
dlmarion commented on code in PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#discussion_r851251266
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ServerClient.java:
##########
@@ -88,6 +88,9 @@ public static <T> T executeRaw(ClientContext context,
public static <CT extends TServiceClient,RT> RT executeRaw(ClientContext context,
TServiceClientFactory<CT> factory, ClientExecReturn<RT,CT> exec) throws Exception {
while (true) {
+ if (Thread.currentThread().isInterrupted()) {
Review Comment:
I made a comment with an alternate solution, but I think you want to call `Thread.interrupted` here as the Threads' interrupted state will be reset (to false) if it's true. https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/lang/Thread.html#interrupted()
--
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 #2622: Handle thread interrupt in server client execute loop.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on PR #2622:
URL: https://github.com/apache/accumulo/pull/2622#issuecomment-1095884144
@nikita-sirohi Thanks for the explanation! It helps a lot.
(Note: I updated your markdown blocks in your comment above, so it looks better... in case you were curious why I edited your comment.)
I'll have to see where this pattern is being used elsewhere... it feels wrong, but if it's being done that elsewhere, I'm curious why. Ideally, if we know that this can manifest as `ClosedByInterruptException` when opening a socket, we should probably have a separate catch block just for that to handle it differently than other `IOException`s, so we don't retry forever and maybe the interrupt exception gets propagated instead.
I'm not sure I understand what you mean about `wasn't sure if either createClientTransport or TTimeoutTransport.create had the appropriate information to special case this exception`, but I suspect this will be obvious if we try to handle it differently.
I'm not familiar with trino. What circumstance with that caused you to see this? Does it just have a command-line interface and you hit `Ctrl+C` while it was opening a socket? Or does it interrupt Accumulo threads routinely by some other mechanism? I know I haven't seen this very often... these threads don't normally get interrupted while opening sockets, so this is a very strange edge case for me.
--
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