[#2254][FOLLOWUP] improvement(spark):Add include key filter before report extraProperties #2265

maobaolong · 2024-11-26T13:03:54Z

What changes were proposed in this pull request?

Add include key filter before report extraProperties
Related docs

Why are the changes needed?

Add include key filter
Add documents.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested on our test cluster.

Client conf

--conf spark.shuffle.manager=org.apache.spark.shuffle.DelegationRssShuffleManager --conf spark.rss.access.id="access.id"
--conf spark.rss.client.reportIncludeProperties="spark.test.key,test2.key,app.id,yarn.queue" --conf spark.rss.client.reportExcludeProperties="app.id"

Coordinator_rpc_audit.log

[2024-11-26 21:14:45.632] cmd=accessCluster	statusCode=ACCESS_DENIED	from=/XXX:34733	executionTimeUs=276(<1s)	appId=N/A	args{accessInfo=AccessInfo{accessId='rss-test-mbl-003-bannedid', user= tdwadmin, tags=[ss_v5], extraProperties={access_info_required_shuffle_nodes_num=-1, yarn.queue=g_teg_tdw_operlog-offline, spark.test.key=123_456}}}

maobaolong · 2024-11-26T13:05:14Z

@jerqi Thanks for the review of #2254, I added the related document and add white list config to improve this feature.

github-actions · 2024-11-26T13:30:08Z

Test Results

2 966 files ± 0 2 966 suites ±0 6h 38m 44s ⏱️ + 11m 24s
1 100 tests + 4 1 098 ✅ + 4 2 💤 ±0 0 ❌ ±0
13 774 runs +39 13 744 ✅ +39 30 💤 ±0 0 ❌ ±0

Results for commit 1de03b7. ± Comparison against base commit db611f4.

♻️ This comment has been updated with latest results.

jerqi · 2024-11-26T14:11:58Z

client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java

    rssConf.getAll().stream()
+        .filter(entry -> includeProperties.isEmpty() || includeProperties.contains(entry.getKey()))


Why do we judge the includeProperties.isEmpty()?

Empty means include all, that means include config is disabled, it reference what hdfs treat include/excludes datanode list

Empty should mean including empty.

So how to express including all?

If we have include list, you can't express including all.

Well, if I set the include option withNoDefaults, do you think it can make sense if we do not set this include option stand for include all?

maobaolong · 2024-12-02T16:37:58Z

@jerqi Sorry for the late reply, it really busy these days, addressed the suggestion from you PTAL.

jerqi · 2024-12-03T02:01:06Z

docs/client_guide/client_guide.md

@@ -60,6 +60,8 @@ The important configuration of client is listed as following. These configuratio
 | <client_type>.rss.client.rpc.netty.maxOrder                     | 3                                      | The value of maxOrder for PooledByteBufAllocator when using gRPC internal Netty on the client-side. This configuration will only take effect when rss.rpc.server.type is set to GRPC_NETTY.                                                                                                                                                                                                                                                                                                  |
 | <client_type>.rss.client.rpc.netty.smallCacheSize               | 1024                                   | The value of smallCacheSize for PooledByteBufAllocator when using gRPC internal Netty on the client-side. This configuration will only take effect when rss.rpc.server.type is set to GRPC_NETTY.                                                                                                                                                                                                                                                                                            |
 | <client_type>.rss.client.blockIdManagerClass                    | -                                      | The block id manager class of server for this application, the implementation of this interface to manage the shuffle block ids                                                                                                                                                                                                                                                                                                                                                              |
+| <client_type>.rss.client.reportExcludeProperties                | -                                      | the report exclude properties could be configured by this option                                                                                                                                                                                                                                                                                                                                                                                                                             |


Is the document right about reportExcludeProperties and reportIncludeProperties?

maobaolong · 2025-01-03T03:37:40Z

@jerqi I updated the document of those new configure keys. PTAL

jerqi

LGTM.

maobaolong · 2025-01-04T06:04:24Z

@jerqi Thanks for your review! Merged.

maobaolong added 2 commits November 26, 2024 20:54

Introduce an include property config list to limit report config size

ca7186c

Add documents

063378c

jerqi reviewed Nov 26, 2024

View reviewed changes

Address comment, set no default value and add UT

a1fb2fd

maobaolong force-pushed the whitelist.gh branch from 7f81482 to a1fb2fd Compare December 2, 2024 17:06

jerqi changed the title ~~[#2254][FOLLOWUP] improvement(spark-client):Add include key filter before report extraProperties~~ [#2254][FOLLOWUP] improvement(spark):Add include key filter before report extraProperties Dec 3, 2024

jerqi reviewed Dec 3, 2024

View reviewed changes

Improve document

1de03b7

maobaolong requested a review from jerqi January 3, 2025 03:35

jerqi approved these changes Jan 3, 2025

View reviewed changes

maobaolong merged commit fd27a08 into apache:master Jan 4, 2025
43 checks passed

		rssConf.getAll().stream()
		.filter(entry -> includeProperties.isEmpty() \|\| includeProperties.contains(entry.getKey()))

[#2254][FOLLOWUP] improvement(spark):Add include key filter before report extraProperties #2265

[#2254][FOLLOWUP] improvement(spark):Add include key filter before report extraProperties #2265

Uh oh!

Conversation

maobaolong commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maobaolong commented Nov 26, 2024

Uh oh!

github-actions bot commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maobaolong commented Dec 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maobaolong commented Jan 3, 2025

Uh oh!

jerqi left a comment

Choose a reason for hiding this comment

Uh oh!

maobaolong commented Jan 4, 2025

Uh oh!

Uh oh!

Uh oh!

maobaolong commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Nov 26, 2024 •

edited

Loading