You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "lwllvyb (via GitHub)" <gi...@apache.org> on 2024/10/23 13:22:31 UTC
[PR] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
lwllvyb opened a new pull request, #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220
### What changes were proposed in this pull request?
Configure a bitmap for each partition instead of sharing the bitmap for multiple partitions
### Why are the changes needed?
Fix: #2219
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Tested locally.
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#issuecomment-2432266759
## Test Results
2 895 files - 31 2 895 suites - 31 6h 5m 11s ⏱️ - 9m 8s
1 088 tests ± 0 1 084 ✅ - 2 2 💤 ±0 2 ❌ +2
13 570 runs - 60 13 538 ✅ - 62 30 💤 ±0 2 ❌ +2
For more details on these failures, see [this check](https://github.com/apache/incubator-uniffle/runs/31951697761).
Results for commit 1e5cd988. ± Comparison against base commit b5290b21.
[test-results]:data:application/gzip;base64,H4sIALr+GGcC/1WOSw6DIBBAr2JYd8FHytDLNIiQkKo0fFamdy9opHT53sy8zI6sW0xEj4GC5LcBxexSz3MOKjm/VUMkIUWVeaobBANc+IxZ69ONP/dy73rXhFVu+RMmBB+KwcWEvB1VxsWFrco4g+bOKmtLXfXgPqr9urpUABHD9SwB5CSwVrI8emfUEjEpCkKx0VBpsaYWfb6w20H/FAEAAA==
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#issuecomment-2440375453
<img width="1432" alt="image" src="https://github.com/user-attachments/assets/95276666-bd6f-4726-9dc3-5875fe5c3588">
@lwllvyb FYI, this is the maximum partition num with node metrics in our cluster, it is ~410K, it cost heap size 410K * 50KiB = 2GiB if we say a bitmap related a partition avg 10000 blocks(cost 50KiB heap).
I think this is worth to choose this new policy in our production cluster.
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#discussion_r1814333875
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -720,6 +720,12 @@ public class ShuffleServerConf extends RssBaseConf {
.defaultValue(false)
.withDescription("Whether to enable app detail log");
+ public static final ConfigOption<Boolean> SERVER_BITMAP_NEW_STRATEGY_ENABLED =
Review Comment:
Maybe a boolean type is not good for extension, it has only two state `true` or `false`, suggest another choice here is that define a policy interface and two class implementation.
- CombinedBitmapPolicy
- PartitionBitmapPolicy
Than you can set it default to `CombinedBitmapPolicy` to keep consistent with original 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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#issuecomment-2433007660
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2220?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `19.14894%` with `76 lines` in your changes missing coverage. Please review.
> Project coverage is 52.23%. Comparing base [(`34bf686`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/34bf686c8440a1a86738176b5d67440ef16a2134?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`1e5cd98`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/1e5cd9889b70ca9108632f17ba287a34e29f0c2f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 2 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2220?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/uniffle/server/ShuffleTaskManager.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2220?src=pr&el=tree&filepath=server%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Funiffle%2Fserver%2FShuffleTaskManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlVGFza01hbmFnZXIuamF2YQ==) | 14.60% | [70 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2220?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2220 +/- ##
============================================
+ Coverage 51.84% 52.23% +0.39%
- Complexity 2864 2895 +31
============================================
Files 469 461 -8
Lines 23879 21883 -1996
Branches 1966 2017 +51
============================================
- Hits 12380 11431 -949
+ Misses 10726 9719 -1007
+ Partials 773 733 -40
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2220?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?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
Re: [PR] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "lwllvyb (via GitHub)" <gi...@apache.org>.
lwllvyb closed pull request #2220: [#2219] feat(server) add new bitmap strategy
URL: https://github.com/apache/incubator-uniffle/pull/2220
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on code in PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#discussion_r1814336338
##########
server/src/main/java/org/apache/uniffle/server/ShuffleTaskManager.java:
##########
@@ -809,13 +941,23 @@ public void removeResourcesByShuffleIds(String appId, List<Integer> shuffleIds)
taskInfo.getCommitLocks().remove(shuffleId);
}
}
- Optional.ofNullable(partitionsToBlockIds.get(appId))
- .ifPresent(
- x -> {
- for (Integer shuffleId : shuffleIds) {
- x.remove(shuffleId);
- }
- });
+ if (newBitmapStrategyEnabled) {
Review Comment:
It is hard to maintain code with these kind of if...else...else if...
Instead, I strongly recommend to use a policy driven way
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#issuecomment-2444010903
@lwllvyb Thanks for your initial work on this improvement, as this is needed this week, I take it over and do some refactor, see https://github.com/apache/incubator-uniffle/pull/2227/
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #2220:
URL: https://github.com/apache/incubator-uniffle/pull/2220#issuecomment-2445626249
@lwllvyb Close this as this continued by #2227
--
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] [#2219] feat(server) add new bitmap strategy [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong closed pull request #2220: [#2219] feat(server) add new bitmap strategy
URL: https://github.com/apache/incubator-uniffle/pull/2220
--
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