Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maobaolong merged 4 commits into from
Jan 4, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca7186c
Introduce an include property config list to limit report config size
maobaolong Nov 26, 2024
063378c
Add documents
maobaolong Nov 26, 2024
a1fb2fd
Address comment, set no default value and add UT
maobaolong Dec 2, 2024
1de03b7
Improve document
maobaolong Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions client-spark/spark2/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,10 @@ private boolean tryAccessCluster() {
RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
List<String> excludeProperties =
rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
List<String> includeProperties =
rssConf.get(RssClientConf.RSS_CLIENT_REPORT_INCLUDE_PROPERTIES);
rssConf.getAll().stream()
.filter(entry -> includeProperties.isEmpty() || includeProperties.contains(entry.getKey()))

@roryqi roryqi Nov 26, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we judge the includeProperties.isEmpty()?

@maobaolong maobaolong Nov 27, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@roryqi roryqi Nov 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty should mean including empty.

@maobaolong maobaolong Nov 27, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how to express including all?

@roryqi roryqi Nov 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maobaolong maobaolong Nov 27, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@roryqi roryqi Nov 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok.

@maobaolong maobaolong Dec 2, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

.filter(entry -> !excludeProperties.contains(entry.getKey()))
.forEach(entry -> extraProperties.put(entry.getKey(), (String) entry.getValue()));

Expand Down
3 changes: 3 additions & 0 deletions client-spark/spark3/src/main/java/org/apache/spark/shuffle/DelegationRssShuffleManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,10 @@ private boolean tryAccessCluster() {
RssConf rssConf = RssSparkConfig.toRssConf(sparkConf);
List<String> excludeProperties =
rssConf.get(RssClientConf.RSS_CLIENT_REPORT_EXCLUDE_PROPERTIES);
List<String> includeProperties =
rssConf.get(RssClientConf.RSS_CLIENT_REPORT_INCLUDE_PROPERTIES);
rssConf.getAll().stream()
.filter(entry -> includeProperties.isEmpty() || includeProperties.contains(entry.getKey()))
.filter(entry -> !excludeProperties.contains(entry.getKey()))
.forEach(entry -> extraProperties.put(entry.getKey(), (String) entry.getValue()));

Expand Down
7 changes: 7 additions & 0 deletions common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,4 +310,11 @@ public class RssClientConf {
.asList()
.defaultValues()
.withDescription("the report exclude properties could be configured by this option");

public static final ConfigOption<List<String>> RSS_CLIENT_REPORT_INCLUDE_PROPERTIES =
ConfigOptions.key("rss.client.reportIncludeProperties")
.stringType()
.asList()
.defaultValues()
.withDescription("the report include properties could be configured by this option");
}
2 changes: 2 additions & 0 deletions docs/client_guide/client_guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

@roryqi roryqi Dec 3, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the document right about reportExcludeProperties and reportIncludeProperties?

@maobaolong maobaolong Jan 3, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

| <client_type>.rss.client.reportIncludeProperties | - | the report include properties could be configured by this option |

Notice:

Expand Down