Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TEZ-4388: Asynchttpclient can cause stuck TezChild processes #189

Merged
merged 2 commits into from
Feb 22, 2022
Merged

TEZ-4388: Asynchttpclient can cause stuck TezChild processes #189

merged 2 commits into from
Feb 22, 2022

Conversation

abstractdog
Copy link
Contributor

No description provided.

@tez-yetus

This comment was marked as outdated.

Sign in to view
@abstractdog
Copy link
Contributor Author

could you review this @rbalamohan ? I'm hoping that you're still familiar with async http client

@abstractdog
Copy link
Contributor Author

I've just realized this patch NULLs out the static client and forces the next async http connection to create one...I'm not sure this is the perfect solution, let me investigate this further

@abstractdog abstractdog changed the title TEZ-4388: TestSecureShuffle: TezChild processes keep running after test TEZ-4388: Asynchttpclient can cause stuck TezChild processes Feb 17, 2022
@tez-yetus

This comment was marked as outdated.

Sign in to view
rbalamohan
rbalamohan reviewed Feb 22, 2022
View reviewed changes
tez-runtime-library/src/main/java/org/apache/tez/http/async/netty/AsyncHttpConnection.java
TezRuntimeShutdownHandler.addShutdownTask(new Thread(() -> {
try {
if (httpAsyncClient != null) {
httpAsyncClient.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should httpAsyncClient be set to "null" explicitly as well? Without it, it may create issues during container reuse. i.e httpAsyncClient will not be null, but in closed state & system would try to use it leading to exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @rbalamohan, makes sense, fixed

rbalamohan
rbalamohan requested changes Feb 22, 2022
View reviewed changes
Copy link
Contributor

@rbalamohan rbalamohan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left minor comment.

@abstractdog abstractdog force-pushed the TEZ-4388 branch 2 times, most recently from 8ee557a to 27d3f7e Compare February 22, 2022 07:42
@rbalamohan
Copy link
Contributor

LGTM. +1

@tez-yetus

This comment was marked as outdated.

Sign in to view
@tez-yetus

This comment was marked as outdated.

Sign in to view
@tez-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 7s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
-1 ❌ test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
_ master Compile Tests _
+0 🆗 mvndep 5m 6s Maven dependency ordering for branch
+1 💚 mvninstall 9m 15s master passed
+1 💚 compile 1m 33s master passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚 compile 1m 29s master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚 checkstyle 1m 32s master passed
+1 💚 javadoc 1m 36s master passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚 javadoc 1m 23s master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗 spotbugs 1m 0s Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚 findbugs 2m 42s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 10s Maven dependency ordering for patch
+1 💚 mvninstall 0m 50s the patch passed
+1 💚 compile 0m 52s the patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚 javac 0m 52s the patch passed
+1 💚 compile 0m 48s the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚 javac 0m 48s the patch passed
-0 ⚠️ checkstyle 0m 8s tez-common: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0)
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 javadoc 0m 46s the patch passed with JDK Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04
+1 💚 javadoc 0m 42s the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚 findbugs 2m 4s the patch passed
_ Other Tests _
+1 💚 unit 0m 31s tez-common in the patch passed.
+1 💚 unit 0m 34s tez-runtime-internals in the patch passed.
+1 💚 unit 4m 33s tez-runtime-library in the patch passed.
+1 💚 asflicense 0m 33s The patch does not generate ASF License warnings.
40m 3s
Subsystem Report/Notes
Docker ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-189/6/artifact/out/Dockerfile
GITHUB PR #189
JIRA Issue TEZ-4388
Optional Tests dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname Linux 4d37eae13121 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality personality/tez.sh
git revision master / 5241f58
Default Java Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.13+8-Ubuntu-0ubuntu1.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
checkstyle https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-189/6/artifact/out/diff-checkstyle-tez-common.txt
Test Results https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-189/6/testReport/
Max. process+thread count 1099 (vs. ulimit of 5500)
modules C: tez-common tez-runtime-internals tez-runtime-library U: .
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-189/6/console
versions git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@abstractdog abstractdog merged commit fa74b95 into apache:master Feb 22, 2022
asfgit pushed a commit that referenced this pull request Feb 22, 2022
@abstractdog
TEZ-4388: Asynchttpclient can cause stuck TezChild processes (#189) (…
75f753e 
…Laszlo Bodor reviewed by Rajesh Balamohan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rbalamohan rbalamohan Awaiting requested review from rbalamohan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@abstractdog @tez-yetus @rbalamohan