[#1538] feat(spark): report blockIds to spark driver optionally #1677
Conversation
| import org.apache.uniffle.common.util.RssUtils; | ||
|
|
||
| /** The class is to manage the shuffle data blockIds in spark driver side. */ | ||
| public class BlockIdManager { |
There was a problem hiding this comment.
I have a little concern about performance. We ever stored all block ids in one shuffle server. It has poor performance. Because bitmap has poor performance.
There was a problem hiding this comment.
Yes, I will try this and apply this into production to check the performance
| @@ -37,6 +37,12 @@ | |||
|
|
|||
| public class RssSparkConfig { | |||
|
|
|||
| public static final ConfigOption<Boolean> RSS_CLIENT_BLOCK_ID_SELF_MANAGED_ENABLED = | |||
| ConfigOptions.key("rss.client.blockId.selfManagedEnabled") | |||
There was a problem hiding this comment.
selfmanaged is adjective. Here the noun is more proper. 👍
There was a problem hiding this comment.
Maybe we should remove client word to align other blockId config options.
Except, I'm a little worried about the config options about Uniffle. Because there are some less elegant issues. client seems extra. Don't use camel style. Proper segments.
There was a problem hiding this comment.
Maybe we should remove client word to align other blockId config options.
Done
Except, I'm a little worried about the config options about Uniffle. Because there are some less elegant issues. client seems extra. Don't use camel style. Proper segments.
This could be optimized because we have the discard mechanism in configOption, that need to be detailed in doc.
...test/spark3/src/test/java/org/apache/uniffle/test/ContinuousSelectPartitionStrategyTest.java
Outdated
Show resolved
Hide resolved
client-spark/spark2/src/main/java/org/apache/spark/shuffle/RssShuffleManager.java
Show resolved
Hide resolved
| @@ -244,7 +244,7 @@ public int compare(String o1, String o2) { | |||
| .get(RSS_CLIENT_BLOCK_ID_SELF_MANAGED_ENABLED); | |||
| if (ClientType.GRPC == clientType && !blockIdSelfManagedEnabled) { | |||
| // TODO skip validating for GRPC_NETTY, needs to mock ShuffleServerNettyHandler | |||
| // TODO: skip validating when blockId is managed in spark driver side. | |||
| // skip validating when blockId is managed in spark driver side. | |||
There was a problem hiding this comment.
Maybe you can remove the TODO in the 246 line, too.
There was a problem hiding this comment.
No. todo in 246 is reasonable.
|
You need to add a document for this. |
Yes I will if you have other comments. |
This should wait some time 😁 |
|
Minor update. PTAL @jerqi |
I think I will merge this. When this feature is appied in our internal cluster, the performance report will be attached. |
Ok for me, too. |
What changes were proposed in this pull request?
Support report blockIds from shuffle-servers to spark driver optionally
Why are the changes needed?
Fix: #1538
Does this PR introduce any user-facing change?
Yes.
rss.client.blockId.selfManagedEnabledis introduced, default value is false.How was this patch tested?
Integration tests.