HADOOP-19061 Capture exception from rpcRequestSender.start() in IPC.Connection.run() #6519
Conversation
|
Why doesn't the OOM cause the client to fail with the existing code on trunk, i.e. where is the OOM suppressed? After your fix, what error will the client fail with? I'm worried that by suppressing this OOM (due to thread creation) we will end up with an OOM elsewhere and it won't be easily to trace when we have two many open connections. |
It is not suppressed/captured at all: it caused the Connection thread to crash. That is why we don't see Connection thread in our thread dump.
I made slight change to my PR, to capture this exception but also throw the exception after we do some cleanup and remove this Connection object from IPC.client.connections pool. So, the original code would keep the bad Connection object around when the Connection thread crashes (because it does not call close() method). The new code would remove that bad connection object and a new good one will be created next time. |
xinglin
changed the title
|
💔 -1 overall
This message was automatically generated. |
What happens if there is an OOM when creating the new connection? Wondering is we can get into a live-lock. Please do excuse the many questions, I'm not familiar with this code so this is to help my understanding. |
|
💔 -1 overall
This message was automatically generated. |
made some code change. prefer to have a single try-catch block to avoid code duplication. Totally valid question and i'd have to chat with Gobblin team to understand more about their use case and confirm whether this PR would indeed mitigate their job hangings. If we remain low on memory, we will keep getting OOM and yes, we will get into live-lock. However, the difference is in the original implementation, if we ever hit a single OOM, we will end up with a bad connection for this JVM permanently and all RPC requests to that destination will be blocked forever. For gobblin, they'd have to manually delete that JVM (and recreate a new one). With the new code, we don't have that problem and can tolerate certain degree of OOMs. And the expectation is we won't be hitting OOM all the time. If we are hitting OOM all the time, we should look for memory leak or bump memory/heap size. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Client.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
Change LGTM. It there a way to test this, that isn't too complicated. I'm +1 without the test though. |
|
Thanks Simba for reviewing and approving PR.
To do this, i would assume we need to create the rpcRequestThread externally and start it and then add a new API setRpcRequestThread() to Connection class, to set it to the new thread. Does this sound right? Or we need to add a new constructor for Connection class, to take rpcRequestThread as a parameter. Doesn't sound worth doing this for testing this 'trivial' change.
|
Yes, we'd need a new API to either set the thread, or start it before Connection.run(). I couldn't easily find where the Connection thread is starting. I agree it doesn't seem worth it. |
Search for start() in setupIOStream(). it is there. took me a while to find that too. |
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
I agree, I don't think the added work for a unit test here is needed. +1 from me. Thank you for the patch @xinglin and thank you for the extra review @simbadzina!
…onnection.run() (apache#6519) * HADOOP-19061 - Capture exception from rpcRequestSender.start() in IPC.Connection.run() and proper cleaning is followed if an exception is thrown. --------- Co-authored-by: Xing Lin <[email protected]>
Description of PR
rpcRequestThread.start() can fail due to OOM. This will immediately crash the Connection thread, without removing itself from the connections pool. Then for all following getConnection(remoteid), we will get this bad connection object and all rpc requests will be hanging, because this is a bad connection object, without threads being properly running (Neither Connection or Connection.rpcRequestSender thread is running due to OOM.).
In this PR, we moved the rpcRequestThread.start() to be within the try{}-catch{} block, to capture OOM from rpcRequestThread.start() and proper cleaning is followed if we hit OOM.
How was this patch tested?
trivial change. let jenkins build.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?