[Improvement] Support skip memory data when use multiple replicas #400

xianjingfeng · 2022-12-12T11:09:44Z

What changes were proposed in this pull request?

Support filter data when use multiple replica

Why are the changes needed?

Now, filter the data that the client had read from the first replica is not supported when we use multiple replica. This will cause duplicate data to be read.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

jerqi · 2022-12-12T12:19:19Z

Maybe we should remove BLOCK_RANGE filter first.

# Conflicts: # client/src/main/java/org/apache/uniffle/client/impl/ShuffleReadClientImpl.java # client/src/main/java/org/apache/uniffle/client/request/CreateShuffleReadClientRequest.java # common/src/main/java/org/apache/uniffle/common/util/RssUtils.java # integration-test/common/src/test/java/org/apache/uniffle/test/ShuffleServerWithMemoryTest.java # storage/src/main/java/org/apache/uniffle/storage/factory/ShuffleHandlerFactory.java # storage/src/main/java/org/apache/uniffle/storage/handler/impl/MemoryClientReadHandler.java

jerqi · 2022-12-13T03:26:44Z

Could we add a ut for multi replica filter?

jerqi · 2022-12-13T06:09:40Z

storage/src/main/java/org/apache/uniffle/storage/handler/impl/MemoryClientReadHandler.java

@@ -86,7 +108,7 @@ public ShuffleDataResult readShuffleData() {
        partitionId,
        lastBlockId,
        readBufferSize,
-        expectedTaskIdsBitmapFilterEnable ? expectTaskIds : null


Why do we remove expectedTaskIdsBitmapFilterEnable?

If expectedTaskIdsBitmapFilterEnable=false, expectTaskIds will be null

jerqi · 2022-12-13T06:19:41Z

storage/src/main/java/org/apache/uniffle/storage/handler/impl/MemoryClientReadHandler.java

-      ShuffleServerClient shuffleServerClient) {
+      ShuffleServerClient shuffleServerClient,
+      Roaring64NavigableMap expectBlockIds,
+      Roaring64NavigableMap processBlockIds) {


Could we lazily create memory handler?

like

incubator-uniffle/storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java

Line 39 in 4b3d37d

private Callable<ClientReadHandler> hotHandlerCreator;

We can create the handler when we need to use them and the memory handler don't need to care how to construct taskbitmap filter.

Could we lazily create memory handler?

Yes. Other handlers can be lazily created too. Should we raise a new pr?

I think this pr need the lazy creation of memory handler. It will help us simplify the logic of this pr. You can implement it in another pr. And we merge that pr first . It's also ok to implement them in this pr at the same time.

OK, i will do it in this pr

codecov-commenter · 2022-12-13T06:22:56Z

Codecov Report

Merging #400 (1d85413) into master (66c752f) will decrease coverage by 0.93%.
The diff coverage is 2.22%.

@@             Coverage Diff              @@
##             master     #400      +/-   ##
============================================
- Coverage     58.54%   57.60%   -0.94%     
+ Complexity     1611     1393     -218     
============================================
  Files           195      173      -22     
  Lines         11035     9223    -1812     
  Branches        966      816     -150     
============================================
- Hits           6460     5313    -1147     
+ Misses         4195     3573     -622     
+ Partials        380      337      -43

Impacted Files	Coverage Δ
...client/request/CreateShuffleReadClientRequest.java	`0.00% <0.00%> (ø)`
...rg/apache/uniffle/client/util/DefaultIdHelper.java	`100.00% <ø> (ø)`
.../java/org/apache/uniffle/common/util/RssUtils.java	`58.45% <0.00%> (-2.14%)`	⬇️
...uniffle/storage/factory/ShuffleHandlerFactory.java	`0.00% <0.00%> (ø)`
...torage/handler/impl/ComposedClientReadHandler.java	`0.00% <0.00%> (ø)`
.../storage/handler/impl/MemoryClientReadHandler.java	`0.00% <0.00%> (ø)`
...orage/request/CreateShuffleReadHandlerRequest.java	`0.00% <0.00%> (ø)`
...che/uniffle/client/impl/ShuffleReadClientImpl.java	`88.57% <100.00%> (+0.10%)`	⬆️
.../java/org/apache/uniffle/server/ShuffleServer.java	`64.49% <0.00%> (-2.90%)`	⬇️
... and 22 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jerqi · 2022-12-14T03:43:54Z

storage/src/main/java/org/apache/uniffle/storage/handler/impl/MemoryClientReadHandler.java

    this.appId = appId;
    this.shuffleId = shuffleId;
    this.partitionId = partitionId;
    this.readBufferSize = readBufferSize;
    this.shuffleServerClient = shuffleServerClient;
-    this.expectTaskIds = expectTaskIds;
-    this.expectedTaskIdsBitmapFilterEnable = expectedTaskIdsBitmapFilterEnable;
+    if (expectedTaskIdsBitmapFilterEnable) {


Could we calculate taskBitmap in the upper layer? Could we remove expectBlockIds and processBlockIds? It's enough to pass the paramter expectTaskIds?

jerqi · 2022-12-14T06:15:58Z

storage/src/main/java/org/apache/uniffle/storage/factory/ShuffleHandlerFactory.java

+      realExceptBlockIds.xor(request.getProcessBlockIds());
+      expectTaskIds = RssUtils.generateTaskIdBitMap(realExceptBlockIds, request.getIdHelper());
+    } else {
+      expectTaskIds = request.getExpectTaskIds();


Why should epectTaskIds be request#getExpectTaskIds() instead of null?

jerqi · 2022-12-14T06:19:43Z

storage/src/main/java/org/apache/uniffle/storage/factory/ShuffleHandlerFactory.java

@@ -116,14 +119,21 @@ public ClientReadHandler createSingleReplicaClientReadHandler(CreateShuffleReadH
  private ClientReadHandler getMemoryClientReadHandler(CreateShuffleReadHandlerRequest request, ShuffleServerInfo ssi) {
    ShuffleServerClient shuffleServerClient = ShuffleServerClientFactory.getInstance().getShuffleServerClient(
        ClientType.GRPC.name(), ssi);
+    Roaring64NavigableMap expectTaskIds;
+    if (request.isExpectedTaskIdsBitmapFilterEnable()) {


In current implement, we only enable taskbitmapFilter when the partition is skewed. Could we turn on the filter condition for only multi replicas?

jerqi · 2022-12-14T07:21:32Z

storage/src/main/java/org/apache/uniffle/storage/factory/ShuffleHandlerFactory.java

-    Roaring64NavigableMap expectTaskIds;
-    if (request.isExpectedTaskIdsBitmapFilterEnable()) {
+    Roaring64NavigableMap expectTaskIds = null;
+    if (request.isExpectedTaskIdsBitmapFilterEnable() || request.getShuffleServerInfoList().size() > 1) {


Could we put the similar logic in the same place? It's more easy to manage.

incubator-uniffle/client-spark/spark3/src/main/java/org/apache/spark/shuffle/reader/RssShuffleReader.java

Line 125 in 66c752f

this.expectedTaskIdsBitmapFilterEnable = !(mapStartIndex == 0 && mapEndIndex == Integer.MAX_VALUE);

jerqi

LGTM, let @zuston take a look.

zuston

LGTM.

TASK_BITMAP support multiple replicas

6de2d19

xianjingfeng added 2 commits December 13, 2022 10:09

fix spotbugs

0598e0c

xianjingfeng changed the title ~~[Improvement] TASK_BITMAP support multiple replicas~~ [Improvement] Support skip memory data when use multiple replicas Dec 13, 2022

Add UT

b6e3b49

jerqi reviewed Dec 13, 2022

View reviewed changes

Lazily create read handlers

d36b3fd

jerqi reviewed Dec 14, 2022

View reviewed changes

xianjingfeng added 2 commits December 14, 2022 12:21

Optimize

a0ef4e9

Merge branch 'master' into taskbitmap_multireplica

46383d0

jerqi reviewed Dec 14, 2022

View reviewed changes

Fix bug

fcbca52

jerqi reviewed Dec 14, 2022

View reviewed changes

Optimize

1d85413

jerqi approved these changes Dec 14, 2022

View reviewed changes

zuston approved these changes Dec 14, 2022

View reviewed changes

zuston merged commit d47f703 into apache:master Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Improvement] Support skip memory data when use multiple replicas #400

[Improvement] Support skip memory data when use multiple replicas #400

xianjingfeng commented Dec 12, 2022 •

edited

Loading

jerqi commented Dec 12, 2022

jerqi commented Dec 13, 2022

jerqi Dec 13, 2022

xianjingfeng Dec 13, 2022

jerqi Dec 13, 2022

jerqi Dec 13, 2022

jerqi Dec 13, 2022

xianjingfeng Dec 13, 2022

jerqi Dec 13, 2022

xianjingfeng Dec 13, 2022

codecov-commenter commented Dec 13, 2022 •

edited

Loading

jerqi Dec 14, 2022

jerqi Dec 14, 2022

xianjingfeng Dec 14, 2022

jerqi Dec 14, 2022

xianjingfeng Dec 14, 2022

jerqi Dec 14, 2022

jerqi left a comment

zuston left a comment

[Improvement] Support skip memory data when use multiple replicas #400

[Improvement] Support skip memory data when use multiple replicas #400

Conversation

xianjingfeng commented Dec 12, 2022 • edited Loading

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?

jerqi commented Dec 12, 2022

jerqi commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 13, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi left a comment

Choose a reason for hiding this comment

zuston left a comment

Choose a reason for hiding this comment

xianjingfeng commented Dec 12, 2022 •

edited

Loading

codecov-commenter commented Dec 13, 2022 •

edited

Loading