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/11/18 08:43:26 UTC

[PR] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

maobaolong opened a new pull request, #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254

   ### What changes were proposed in this pull request?
   
   put sparkConf as extra properties while client request accessCluster
   
   ### Why are the changes needed?
   
   Coordinator can let the spark application access rss cluster or not by some customized access checker leverage some of the spark configs.
   
   ### Does this PR introduce _any_ user-facing change?
   
   No.
   
   ### How was this patch tested?
   
   No need
   


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1856456356


##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -124,6 +127,16 @@ private boolean tryAccessCluster() {
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
 
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    rssConf.getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   Should you remove `<100`?



##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -124,6 +127,16 @@ private boolean tryAccessCluster() {
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
 
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
+    // Put all spark conf into extra properties, except which length is longer than 100

Review Comment:
   Should you remove the comment?



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2497756471

   ## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?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 `53.84615%` with `6 lines` in your changes missing coverage. Please review.
   > Project coverage is 52.36%. Comparing base [(`477bc30`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/477bc3037081d877317e7de27ad87e3dd42faee2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`9a3b25d`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/9a3b25dba5ac723017b33280bde1d7d9743dfab0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 9 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [.../java/org/apache/spark/shuffle/RssSparkConfig.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?src=pr&el=tree&filepath=client-spark%2Fcommon%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fspark%2Fshuffle%2FRssSparkConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya0NvbmZpZy5qYXZh) | 20.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   | [...uniffle/shuffle/manager/RssShuffleManagerBase.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?src=pr&el=tree&filepath=client-spark%2Fcommon%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Funiffle%2Fshuffle%2Fmanager%2FRssShuffleManagerBase.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zaHVmZmxlL21hbmFnZXIvUnNzU2h1ZmZsZU1hbmFnZXJCYXNlLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?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    #2254      +/-   ##
   ============================================
   - Coverage     52.65%   52.36%   -0.29%     
   + Complexity     3376     2969     -407     
   ============================================
     Files           514      466      -48     
     Lines         27565    22326    -5239     
     Branches       2582     2044     -538     
   ============================================
   - Hits          14513    11691    -2822     
   + Misses        12092     9890    -2202     
   + Partials        960      745     -215     
   ```
   
   </details>
   
   [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/2254?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).
   
   ----
   🚨 Try these New Features:
   
   - [Flaky Tests Detection](https://docs.codecov.com/docs/test-result-ingestion-beta) - Detect and resolve failed and flaky tests
   - [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis) - Avoid shipping oversized bundles


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1856504410


##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -124,6 +127,16 @@ private boolean tryAccessCluster() {
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
 
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    rssConf.getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   Done



##########
client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -124,6 +127,16 @@ private boolean tryAccessCluster() {
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
 
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
+    // Put all spark conf into extra properties, except which length is longer than 100

Review Comment:
   Done



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1856347269


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,10 +126,15 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);

Review Comment:
   Thanks for your tip, I set a default value.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1853694064


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   This is too tricky....



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2496983059

   @jerqi Exclude config added, PTAL


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1848204061


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,7 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    extraProperties.putAll(RssSparkConfig.sparkConfToMap(sparkConf));

Review Comment:
   It's not common if bannedIdProvider contains spark.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1856349782


##########
common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java:
##########
@@ -307,6 +307,6 @@ public class RssClientConf {
       ConfigOptions.key("rss.client.reportExcludeProperties")
           .stringType()
           .asList()
-          .noDefaultValue()
+          .defaultValues("hdfs.inputPaths")

Review Comment:
   Em... this is a weird value. 



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2500135424

   @jerqi sorry for that， will add it this week.


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong merged PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1847573951


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,7 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    extraProperties.putAll(RssSparkConfig.sparkConfToMap(sparkConf));

Review Comment:
   We shouldn't put Spark configurations to coordinator. Coordinator isn't related to compute framework. The spark configurations is coupled with Spark framework. You can have an extractor instead of passing them directly.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1855715892


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   Ok for me, too. `black list` would better to be replaced by `excludeList`.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1855910574


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   done



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1855714732


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   Do you think use black list instead of white list can be possible? 



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1857689506


##########
common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java:
##########
@@ -303,4 +303,10 @@ public class RssClientConf {
           .withDescription(
               "The block id manager class of server for this application, "
                   + "the implementation of this interface to manage the shuffle block ids");
+  public static final ConfigOption<List<String>> RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES =

Review Comment:
   Add a blank line.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1853872894


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   Em... Thanks for your review! I'm sorry for this.
   
   I would greatly appreciate it if you could give me some advice.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2499883072

   We forgot documents.


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1855701365


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   You can implement a white list or configuration filter. The white list can be configured in the configuration. The configuration filter could have length configuration filter. The length could be configured in the configuration.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1857878289


##########
common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java:
##########
@@ -303,4 +303,10 @@ public class RssClientConf {
           .withDescription(
               "The block id manager class of server for this application, "
                   + "the implementation of this interface to manage the shuffle block ids");
+  public static final ConfigOption<List<String>> RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES =

Review Comment:
   done



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2488867095

   @jerqi Thanks for your suggestion, removed the `spark.` prefix to be more generic by new commit 


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1847637096


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,7 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    extraProperties.putAll(RssSparkConfig.sparkConfToMap(sparkConf));

Review Comment:
   This PR is related to https://github.com/apache/incubator-uniffle/pull/2255 , I want to determine whether to ban an app by a specific `ban id` which extract from one of the spark client config, but I don't want client config it concretely.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1848196037


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,7 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    extraProperties.putAll(RssSparkConfig.sparkConfToMap(sparkConf));

Review Comment:
   I'm not sure whether it is good design to have a ban id list. It seems that it isn't common enough.



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.

github-actions[bot] commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2482359764

   ## Test Results
    2 950 files  +25   2 950 suites  +25   5h 59m 1s ⏱️ -48s
    1 092 tests ± 0   1 089 ✅ ± 0   2 💤 ±0  1 ❌ ±0 
   13 673 runs  +48  13 642 ✅ +48  30 💤 ±0  1 ❌ ±0 
   
   For more details on these failures, see [this check](https://github.com/apache/incubator-uniffle/runs/33125298322).
   
   Results for commit 827ee04a. ± Comparison against base commit 3cf82d70.
   
   [test-results]:data:application/gzip;base64,H4sIAJ4EO2cC/02NSQ7DIAwAvxJx7iHgEHA/U1ksEmqWisAp6t8LaUNznLE93pkPk9vYvRMo+1vHthzSlW2OlMK6VMPlwIsq81Q3eI/ixMeWjTmcxr97hle9a8JTmOpSEy7GNRZTP8W8HFUYFfywVWEcRHPfKpw31+rB16hZ5zmkAkwL5Vw/EFnJCZxXisiPEgVY1KDROFIGrWHvD3bsz/YUAQAA
   


-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "maobaolong (via GitHub)" <gi...@apache.org>.

maobaolong commented on PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#issuecomment-2499877214

   @jerqi Thanks for your review! merge 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

Re: [PR] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1855952648


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,10 +126,15 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+
+    RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
+    List<String> excludeProperties =
+        rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);

Review Comment:
   The default is null? Will it cause an error?



-- 
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] [MINOR] improvement(spark-client): put sparkConf as extra properties while client request accessCluster [incubator-uniffle]

Posted by "jerqi (via GitHub)" <gi...@apache.org>.

jerqi commented on code in PR #2254:
URL: https://github.com/apache/incubator-uniffle/pull/2254#discussion_r1855715892


##########
client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java:
##########
@@ -123,6 +123,11 @@ private boolean tryAccessCluster() {
     Map<String, String> extraProperties = Maps.newHashMap();
     extraProperties.put(
         ACCESS_INFO_REQUIRED_SHUFFLE_NODES_NUM, String.valueOf(assignmentShuffleNodesNum));
+    // Put all spark conf into extra properties, except which length is longer than 100
+    // to avoid extra properties too long.
+    RssSparkConfig.toRssConf(sparkConf).getAll().stream()
+        .filter(entry -> StringUtils.length((String) entry.getValue()) < 100)

Review Comment:
   Ok for me, 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