You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "zhuyaogai (via GitHub)" <gi...@apache.org> on 2023/08/06 13:47:08 UTC
[GitHub] [incubator-uniffle] zhuyaogai opened a new pull request, #1102: [#1101] Improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
zhuyaogai opened a new pull request, #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102
<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
Examples:
- "[#123] feat(operator): support xxx"
- "[#233] fix: check null before access result in xxx"
- "[MINOR] refactor: fix typo in variable name"
- "[MINOR] docs: fix typo in README"
- "[#255] test: fix flaky test NameOfTheTest"
Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. Contributor guidelines:
https://github.com/apache/incubator-uniffle/blob/master/CONTRIBUTING.md
3. If the PR is unfinished, please mark this PR as draft.
-->
### What changes were proposed in this pull request?
Only when the JVM is about to exit, the server resources will be released. In some scenarios, it will occupy server resources for a long time. For example, when enable history logging service and timeline service.
### Why are the changes needed?
Fix: https://github.com/apache/incubator-uniffle/issues/1101
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing tests can cover 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
[GitHub] [incubator-uniffle] zhuyaogai commented on a diff in pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "zhuyaogai (via GitHub)" <gi...@apache.org>.
zhuyaogai commented on code in PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#discussion_r1285233835
##########
client-tez/src/main/java/org/apache/tez/dag/app/RssDAGAppMaster.java:
##########
@@ -376,33 +399,6 @@ static void mayCloseTezSlowStart(Configuration conf) {
}
}
- static class RssDAGAppMasterShutdownHook implements Runnable {
Review Comment:
The code logic in this part has been moved to the `serviceStop` method, and it does not need to be executed repeatedly. Therefore, I remove this `ShutdownHook`.
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#discussion_r1285234934
##########
client-tez/src/main/java/org/apache/tez/dag/app/RssDAGAppMaster.java:
##########
@@ -376,33 +399,6 @@ static void mayCloseTezSlowStart(Configuration conf) {
}
}
- static class RssDAGAppMasterShutdownHook implements Runnable {
Review Comment:
> The code logic in this part has been moved to the `serviceStop` method, and it does not need to be executed repeatedly. Therefore, I remove this `ShutdownHook`.
If RssDAGAppMaster exit accidently, it will only execute the logic of `ShutdownHook`, it won't execute the logic of `serviceStop`, it may cause the hang of the JVM.
--
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
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#issuecomment-1666888636
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1102?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#1102](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1102?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e82d342) into [master](https://app.codecov.io/gh/apache/incubator-uniffle/commit/1e4804b944bd91887748fc0ad2771825752f3a28?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1e4804b) will **increase** coverage by `0.91%`.
> Report is 3 commits behind head on master.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #1102 +/- ##
============================================
+ Coverage 54.22% 55.14% +0.91%
+ Complexity 2553 2081 -472
============================================
Files 386 314 -72
Lines 21871 14018 -7853
Branches 1812 1259 -553
============================================
- Hits 11860 7730 -4130
+ Misses 9312 5873 -3439
+ Partials 699 415 -284
```
[see 85 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1102/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [incubator-uniffle] jerqi commented on a diff in pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#discussion_r1285229228
##########
client-tez/src/main/java/org/apache/tez/dag/app/RssDAGAppMaster.java:
##########
@@ -376,33 +399,6 @@ static void mayCloseTezSlowStart(Configuration conf) {
}
}
- static class RssDAGAppMasterShutdownHook implements Runnable {
Review Comment:
Why do we remove this `ShutdownHook`?
--
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
[GitHub] [incubator-uniffle] zhengchenyu commented on pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "zhengchenyu (via GitHub)" <gi...@apache.org>.
zhengchenyu commented on PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#issuecomment-1667554890
It is a great PR! We should release rss resource firstly, especially when timeline is slow.
As the app will be closed, can we call unregisterShuffle explicit? By this way, shuffle server do not need to wait the application timeout. @jerqi Can you give some suggestion?
--
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
[GitHub] [incubator-uniffle] jerqi commented on pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#issuecomment-1666886329
@zhengchenyu Could you help me review this 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: 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
[GitHub] [incubator-uniffle] zhuyaogai commented on a diff in pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "zhuyaogai (via GitHub)" <gi...@apache.org>.
zhuyaogai commented on code in PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#discussion_r1285233835
##########
client-tez/src/main/java/org/apache/tez/dag/app/RssDAGAppMaster.java:
##########
@@ -376,33 +399,6 @@ static void mayCloseTezSlowStart(Configuration conf) {
}
}
- static class RssDAGAppMasterShutdownHook implements Runnable {
Review Comment:
The code logic in this part has been moved to the `serviceStop` method, and it does not need to be executed repeatedly. Therefore, I have deleted 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
[GitHub] [incubator-uniffle] jerqi commented on pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102#issuecomment-1667618798
> It is a great PR! We should release rss resource firstly, especially when timeline is slow. As the app will be closed, can we call unregisterShuffle explicit? By this way, shuffle server do not need to wait the application timeout. @jerqi Can you give some suggestion?
@zhengchenyu It's ok for me. It can be another pr, you can create an issue for it.
For this pr, let me merge it first.
--
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
[GitHub] [incubator-uniffle] jerqi merged pull request #1102: [#1101] improvement(tez): Release server resources as soon as possible in RssDAGAppMaster.
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #1102:
URL: https://github.com/apache/incubator-uniffle/pull/1102
--
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