You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "maobaolong (via GitHub)" <gi...@apache.org> on 2024/07/10 02:17:06 UTC
[PR] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
maobaolong opened a new pull request, #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882
<!--
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?
Make spark client uniifle class under org.apache.uniffle package to avoid conflict
### Why are the changes needed?
(Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.)
Fix: #1881
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
- Tests on local
- Tests on our test 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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2220246329
This is a breaking change, too.
--
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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2220233373
<img width="811" alt="image" src="https://github.com/apache/incubator-uniffle/assets/17329931/41de2b65-d3b5-404b-99c1-0655c88e79ae">
@jerqi Thanks for your explain, but in the other side, we could meet `MethodNotDefException` future in the following scenario:
- If spark create a class have the same package and class name with the above screenshot, we could use a conflict class.
- If some other dependencies have the same class and package name, we could use a conflict class too.
So, I have to suggest to make sure all of our uniffle client classes are under `org.apache.uniffle` package or relocated by shaded maven plugins, if we do like this, we can reduce trouble of `conflict class`.
--
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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2219387095
## Test Results
2 524 files - 133 2 524 suites - 133 3h 44m 18s :stopwatch: - 1h 46m 50s
934 tests - 12 906 :white_check_mark: - 39 1 :zzz: ±0 0 :x: ±0 27 :fire: + 27
11 594 runs - 205 11 363 :white_check_mark: - 421 15 :zzz: ±0 0 :x: ±0 216 :fire: +216
For more details on these errors, see [this check](https://github.com/apache/incubator-uniffle/runs/27247369274).
Results for commit 005403da. ± Comparison against base commit c48af78e.
<details>
<summary>This pull request <b>removes</b> 73 and <b>adds</b> 61 tests. <i>Note that renamed tests count towards both.</i></summary>
```
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateFallback
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInDriver
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInDriverDenied
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInExecutor
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testTryAccessCluster
org.apache.spark.shuffle.FunctionUtilsTests ‑ testOnceFunction0
org.apache.spark.shuffle.RssShuffleManagerTest ‑ testCreateShuffleManagerServer
org.apache.spark.shuffle.RssShuffleManagerTest ‑ testGetDataDistributionType
org.apache.spark.shuffle.RssShuffleManagerTest ‑ testRssShuffleManagerInterface
org.apache.spark.shuffle.RssShuffleManagerTest ‑ testRssShuffleManagerRegisterShuffle{int}[1]
…
```
```
org.apache.uniffle.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateFallback
org.apache.uniffle.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInDriver
org.apache.uniffle.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInDriverDenied
org.apache.uniffle.spark.shuffle.DelegationRssShuffleManagerTest ‑ testCreateInExecutor
org.apache.uniffle.spark.shuffle.DelegationRssShuffleManagerTest ‑ testTryAccessCluster
org.apache.uniffle.spark.shuffle.FunctionUtilsTests ‑ testOnceFunction0
org.apache.uniffle.spark.shuffle.RssShuffleManagerTest ‑ testCreateShuffleManagerServer
org.apache.uniffle.spark.shuffle.RssShuffleManagerTest ‑ testGetDataDistributionType
org.apache.uniffle.spark.shuffle.RssShuffleManagerTest ‑ testRssShuffleManagerInterface
org.apache.uniffle.spark.shuffle.RssShuffleManagerTest ‑ testRssShuffleManagerRegisterShuffle{int}[1]
…
```
</details>
[test-results]:data:application/gzip;base64,H4sIAHPzjWYC/02PSw6DIBBAr2JYdzHIjEov05jBSUj9NAirpncvNEpdvge8Gd5K/Dzt6t601OKtUXvy8couhTH6bc1GG6Qhq3weyw1r8KTHnpiLgu6vnv5VXlUho5+zgCqmELZQRvVZhbSWqNZk8cCzqrXpTHVHlk5xyf64VnXZhbdl8TGjAiAE40aRSYiF0TKQFXZDjy5/tkO2QD2ozxeOgYeNFQEAAA==
--
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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2220373894
@jerqi I see. So how about another resolution, we do not add a uniffle pacakge name, instead, we add a class prefix with Uniffle. e.g. `SparkVersionUtils` -> `UnifleSparkVersionUtils` ?
BTW, I'm open to keep it like it is now, we can merge this PR after we first encounter a class conflict issue involved by this.
--
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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2219367040
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1882?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 53.11%. Comparing base [(`704c70c`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/704c70c3ca9e33e195708e3a93067438ea12aa9a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`005403d`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/005403daffef5cfc49c059fcd874d52464c90570?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.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1882 +/- ##
============================================
- Coverage 53.35% 53.11% -0.24%
+ Complexity 2846 2843 -3
============================================
Files 428 433 +5
Lines 22805 22915 +110
Branches 2142 2141 -1
============================================
+ Hits 12167 12171 +4
- Misses 9861 9976 +115
+ Partials 777 768 -9
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1882?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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "maobaolong (via GitHub)" <gi...@apache.org>.
maobaolong commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2259488085
@jerqi I study celeborn, css(byteDance), shuttle(oppo) , just paste some thoughts from my point. All of them put the client class entry point and related classes under a specific package, they are celeborn/css/shuttle, but for uniffle, we put it into `shuffle` package and named like `RssXXX`, which is too generic.
- celeborn <img width="1361" alt="image" src="https://github.com/user-attachments/assets/5e50aa95-6f1a-4325-8bb9-25a2e53f3f5f">
- css https://github.com/bytedance/CloudShuffleService/blob/main/spark-shuffle-manager-3/src/main/scala/org/apache/spark/shuffle/css/CssShuffleManager.scala
- shuttle https://github.com/cubefs/shuttle/blob/master/src/main/scala/org/apache/spark/shuffle/Ors2ShuffleManager.scala
--
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] [#1881] fix(client-spark): Make spark client uniifle class under org.apache.uniffle package to avoid conflict [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1882:
URL: https://github.com/apache/incubator-uniffle/pull/1882#issuecomment-2219572148
Why do we them in one package? Because we don't use default package method easily.
--
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