You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "EnricoMi (via GitHub)" <gi...@apache.org> on 2024/05/13 11:30:53 UTC
[PR] Dont log some errors on exit [incubator-uniffle]
EnricoMi opened a new pull request, #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714
### What changes were proposed in this pull request?
Once `ShuffleWriteClientImpl.close()` / `RegisterHeartBeat.shutdown()` is called, failing async calls should not be logged as `WARN` log level as those calls are expected to fail in that situation.
### Why are the changes needed?
Some async calls may not have finished on close / shutdown, which raises warnings in the log. This pollutes logs and makes tracing unrelated issues harder:
```
WARN impl.ShuffleWriteClientImpl: Error happened when unregistering to ShuffleServerInfo{host[10.109.9.45], grpc port[19999]}
io.grpc.StatusRuntimeException: CANCELLED: Thread interrupted
at io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:268)
at io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:249)
at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:167)
at org.apache.uniffle.proto.ShuffleServerGrpc$ShuffleServerBlockingStub.unregisterShuffleByAppId(ShuffleServerGrpc.java:772)
at org.apache.uniffle.client.impl.grpc.ShuffleServerGrpcClient.doUnregisterShuffleByAppId(ShuffleServerGrpcClient.java:345)
at org.apache.uniffle.client.impl.grpc.ShuffleServerGrpcClient.unregisterShuffleByAppId(ShuffleServerGrpcClient.java:352)
at org.apache.uniffle.client.impl.ShuffleWriteClientImpl.lambda$unregisterShuffle$28(ShuffleWriteClientImpl.java:1020)
at org.apache.uniffle.common.util.ThreadUtils.lambda$executeTasks$0(ThreadUtils.java:110)
at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
at java.base/java.lang.Thread.run(Unknown Source)
Caused by: java.lang.InterruptedException
at io.grpc.stub.ClientCalls$ThreadlessExecutor.throwIfInterrupted(ClientCalls.java:750)
at io.grpc.stub.ClientCalls$ThreadlessExecutor.waitAndDrain(ClientCalls.java:718)
at io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:159)
... 9 more
```
Fix: #1713
### Does this PR introduce _any_ user-facing change?
Yes, log messages are more specific on the situation when async calls fail after close / shutdown has been called.
### How was this patch tested?
No.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126394712
> As you said [#1714 (comment)](https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2120490454), it could be JVM shutting down which causes `InterruptedException`. I think this [patch](https://github.com/apache/incubator-uniffle/pull/1736) might work, could you please test it?
By the time `close` is called, all threads are completed. Calling `unregisterExecutorService.awaitTermination()` then is redundant. I'll check if the observed issue is due to too many shuffle servers with to few threads.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126335709
As you said https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2120490454, it could be JVM shutting down which causes `InterruptedException`. I think this [patch](https://github.com/apache/incubator-uniffle/pull/1736) might work, could you please test 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126676535
> > By the time `close` is called, all threads are completed.
>
> If I'm correct, this is not guaranteed. These are different ExecutorServices.
Let me rephrase this: By the time `ShuffleWriteClientImpl.unregisterShuffle` returns, all unregister requests completed.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi closed pull request #1714: [#1713] Improvement: Don't log some errors on exit
URL: https://github.com/apache/incubator-uniffle/pull/1714
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#discussion_r1598341939
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
+ + " after shuffle write client has been closed",
+ e);
+ } else {
+ LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
Review Comment:
For other similar logging issues in this PR, please modify them together as well. :)
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#discussion_r1606609981
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
+ + " after shuffle write client has been closed",
+ e);
+ } else {
+ LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
Review Comment:
There are still a lot of places using `+ shuffleServerInfo`, you can search for them.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2120232452
Do you know the real reason behind the failure of unregistering shuffles? I'm actually quite curious about this. It certainly isn't because of `ShuffleWriteClientImpl.close() / RegisterHeartBeat.shutdown()` being called. Because this would cause the shuffle to be delayed in being cleaned up on the server side, resulting in a waste of resources.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2120490454
> Do you know the real reason behind the failure of unregistering shuffles?
I think it is because the executor JVM is shutting down. It calls `ShuffleManager.stop`, which does not wait on ongoing calls to terminate, as they run in a different thread pool.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2120540788
> I think it is because the executor JVM is shutting down. That might interrupt the waiting threads (`InterruptedException`).
Yeah, what improvement measures can we take to avoid this situation as much as possible?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126506220
> By the time `close` is called, all threads are completed.
If I'm correct, this is not guaranteed. These are different ExecutorServices.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126679597
I think the right thing to do here is to use the timeout for each individual request and to wait for the concurrent requests according to their _sequenciality_ (i.e. the number of sequential requests given the number of threads): #1738
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2158282050
This is a genuine problem, so do not ignore the error. Proper fix is #1766 and #1738.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2120114236
You are right @rickyma. An executor finishes by calling close / shutdown and then may terminate. That interrupts waiting threads and causes ongoing calls to throw exceptions. Since close / shutdown has been called, we don't want those exceptions to be logged as errors / warns.
There are two options
1) wait gracefully for the open requests to complete or timeout, but this defers the executor termination, which might not be desired
2) terminate the executor quickly while relying on clean-up mechanisms (current behaviour)
We don't care about the heartbeats, and shuffles are cleaned-up after some timeout anyway.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#discussion_r1598336937
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
+ + " after shuffle write client has been closed",
+ e);
+ } else {
+ LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
Review Comment:
Ditto.
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
Review Comment:
Also, `when sending` is better.
##########
server/src/main/java/org/apache/uniffle/server/RegisterHeartBeat.java:
##########
@@ -137,7 +140,9 @@ boolean sendHeartBeat(
sendSuccessfully.set(true);
}
} catch (Exception e) {
- LOG.error(e.getMessage());
+ if (!shutdown) {
+ LOG.error(e.getMessage());
Review Comment:
I think `LOG.error("xxx", e);` is better than `LOG.error(e.getMessage());`. Because it will give a full stack trace.
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
Review Comment:
It's better to use `{}` placeholders.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2107632815
BTW, I think `Error happened when unregistering to ShuffleServerInfo` error has nothing to do with `ShuffleWriteClientImpl.close() / RegisterHeartBeat.shutdown()` being called. Because unregistering threads are in a different thread pool which will not be shutdown by calling `ShuffleWriteClientImpl.close() / RegisterHeartBeat.shutdown()`. It might be shutdown when `spark.rss.client.unregister.request.timeout.sec` timeout is reached.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126694397
> Let me rephrase this: By the time `ShuffleWriteClientImpl.unregisterShuffle` returns, all unregister requests completed.
This is right. The method is not asychronized.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2122024921
Waiting for the queue of requests to complete. Is the that desired behaviour?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2122090981
Normally, the executor should shutdown gracefully. `InterruptedException` is not a desired behavior.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2125101022
> @rickyma what about #1736?
The idea looks good to 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#discussion_r1606429837
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
Review Comment:
fixed
##########
server/src/main/java/org/apache/uniffle/server/RegisterHeartBeat.java:
##########
@@ -137,7 +140,9 @@ boolean sendHeartBeat(
sendSuccessfully.set(true);
}
} catch (Exception e) {
- LOG.error(e.getMessage());
+ if (!shutdown) {
+ LOG.error(e.getMessage());
Review Comment:
improved
##########
client/src/main/java/org/apache/uniffle/client/impl/ShuffleWriteClientImpl.java:
##########
@@ -914,7 +915,16 @@ public void sendAppHeartbeat(String appId, long timeoutMs) {
LOG.warn("Failed to send heartbeat to " + shuffleServerInfo);
}
} catch (Exception e) {
- LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
+ if (closed) {
+ // this client has been closed, so failing heartbeat calls are expected
+ LOG.info(
+ "Error happened when send heartbeat to "
+ + shuffleServerInfo
+ + " after shuffle write client has been closed",
+ e);
+ } else {
+ LOG.warn("Error happened when send heartbeat to " + shuffleServerInfo, e);
Review Comment:
done
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2126266185
I have checked the implementation of `ThreadUtils.executeTasks`. That method is meant to return **after** all tasks (unregister requests) have completed, either successfully, with failure, or after timeout. Then, the executor is shutdown so that any ongoing request is being cancelled.
So I am not sure why we are seeing those interrupt exceptions. Either these indicate timeouts or some other cause interrupt.
The `unregisterRequestTimeSec` refers to invoking all unregister requests, not individual requests. If `unregisterThreadPoolSize` is smaller than `shuffleServerInfos.size()`, then some requests are not executed in parallel.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Don't log some errors on exit [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2107392915
## Test Results
2 391 files ±0 2 391 suites ±0 4h 58m 5s :stopwatch: + 1m 19s
928 tests ±0 927 :white_check_mark: ±0 1 :zzz: ±0 0 :x: ±0
10 754 runs ±0 10 740 :white_check_mark: ±0 14 :zzz: ±0 0 :x: ±0
Results for commit 0aa6ac15. ± Comparison against base commit d4a5fb75.
[test-results]:data:application/gzip;base64,H4sIAGwBQmYC/03MTQ6DIBCG4asY1l0wCgK9TMPPmJCqNAgr07sXjVKW38Pw7mTyM27k2fWDgkdHtuxTu12OOvmwFgEhJS9U3tNxoXp5r9eWrT1J/OntP8evCpP2cwFaAWMM8ZKY16MJVHB2zTtajNFqV7UeNdVzt1EblsWnMgjVetQWuJEwGaTOaYYwCINcWQYjR+ccGqvI9wfLHuVsEgEAAA==
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1713] Improvement: Don't log some errors on exit [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #1714:
URL: https://github.com/apache/incubator-uniffle/pull/1714#issuecomment-2124524673
@rickyma what about #1736?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org