weixiuli commented on a change in pull request #31942:
URL: https://github.com/apache/spark/pull/31942#discussion_r600991051
##########
File path:
common/network-common/src/main/java/org/apache/spark/network/client/TransportResponseHandler.java
##########
@@ -188,6 +188,7 @@ public void handle(ResponseMessage message) throws
Exception {
if (listener == null) {
Review comment:
`I wonder if it's better to extend the whole try-finally block around
this else-if branch? what if outstandingRpcs.get/remove fails? highly unlikely
I guess so any 'leak' is trivial.` , Yes , the outstandingRpcs.get/remove can
not fail, so we should not try-finally block around this else-if branch here,
which may Improve performance.
`But what about the same problem around line 168 and 172? there's no
try-finally around onSuccess, which could fail?`, i think we should also
try-finally around onSuccess around line 168 and 172, do you think so ?
`Do all of these messages really need to be release()-ed? meaning, should
.release() happen in all branches?`,It should be released, although this
scenario is not easy to happen.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
