[#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler #570

kaijchen · 2023-02-09T07:33:46Z

What changes were proposed in this pull request?

Add enum Tier, use EnumMap to replace switch-case in ComposedClientReadHandler.

Why are the changes needed?

Make the code more concise.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing CI.

codecov-commenter · 2023-02-09T08:24:01Z

Codecov Report

Merging #570 (3b725d5) into master (d0f57ef) will increase coverage by 0.23%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master     #570      +/-   ##
============================================
+ Coverage     60.88%   61.11%   +0.23%     
- Complexity     1796     1798       +2     
============================================
  Files           213      213              
  Lines         12375    12332      -43     
  Branches       1049     1042       -7     
============================================
+ Hits           7534     7537       +3     
+ Misses         4434     4386      -48     
- Partials        407      409       +2

Impacted Files	Coverage Δ
...uniffle/storage/factory/ShuffleHandlerFactory.java	`0.00% <0.00%> (ø)`
...torage/handler/impl/ComposedClientReadHandler.java	`0.00% <0.00%> (ø)`
...he/uniffle/server/storage/LocalStorageManager.java	`85.87% <0.00%> (-1.13%)`	⬇️
...he/uniffle/server/buffer/ShuffleBufferManager.java	`83.09% <0.00%> (-0.89%)`	⬇️
...rg/apache/uniffle/server/ShuffleServerMetrics.java	`97.34% <0.00%> (+0.02%)`	⬆️
...rg/apache/uniffle/storage/common/LocalStorage.java	`49.05% <0.00%> (+0.69%)`	⬆️

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

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

xianjingfeng · 2023-02-09T08:57:54Z

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

-    }
-    if (topLevelOfHandler > 1) {
-      this.warmDataReadHandler = handlers[1];
+    numTiers = Math.min(Tier.values().length, handlers.length);


If handlers.length > Tier.values().length, should an exception be thrown?

xianjingfeng · 2023-02-09T09:02:37Z

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

+      ClientReadHandler handler = handlerMap.computeIfAbsent(currentTier,
+          key -> supplierMap.getOrDefault(key, () -> null).get());
+      if (handler == null) {
+        return null;


Should an exception be thrown here?

The current logic is equivalent to the old logic.

Yes, but it is unreasonable. It means there is a bug if handler=null. It may increase the difficulty of discover problems.

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

advancedxy · 2023-02-09T09:18:15Z

...ion-test/common/src/test/java/org/apache/uniffle/test/ShuffleServerWithMemLocalHdfsTest.java

-      assert (readBlokNumInfo.contains("Client read 0 blocks from [" + ssi + "]")
-          && readBlokNumInfo.contains("Skipped[ hot:3 warm:3 cold:2 frozen:0 ]")
-          && readBlokNumInfo.contains("Consumed[ hot:0 warm:0 cold:0 frozen:0 ]"));
+      String readBlockNumInfo = composedClientReadHandler.getReadBlockNumInfo();


advancedxy

Thanks @kaijchen ... I thought about to refactor this.
It's much better now.

advancedxy · 2023-02-09T09:29:47Z

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

+  private final Map<Tier, Supplier<ClientReadHandler>> supplierMap = new EnumMap<>(Tier.class);
+  private final Map<Tier, ClientReadHandler> handlerMap = new EnumMap<>(Tier.class);
+  private final Map<Tier, ClientReadHandlerMetric> metricsMap = new EnumMap<>(Tier.class);
+  private Tier currentTier = Tier.values()[0];


this seems a bit strange. Could we just use Hot here?

Just in case of we add some new Super Hot tier before Hot.

I have added a comment here.

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

xianjingfeng · 2023-02-10T02:06:45Z

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

-    if (frozenDataReadHandler != null) {
-      frozenDataReadHandler.close();
-    }
+    handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);


Is filter(Objects::nonNull) necessary?

Not required, but just in case.
And to be consistent with the old logic.

I think we can remove it here, because there is an extra loop. WDYT?

There are at most 4 elements in the map ...

The key is whether it is necessary to keep it. It's OK for me too to keep it.

It's just for safety.

Actually, the values of EnumMap will not be null. If we put null to EnumMap, it will be modified to an instance of Object.

OK, let me check.

Actually, the values of EnumMap will not be null. If we put null to EnumMap, it will be modified to an instance of Object.

No, it could be null. The following test passed.

My mistake. When we get value from EnumMap, it will be modified to null again.

xianjingfeng

LGTM

kaijchen · 2023-02-10T03:51:03Z

Thanks @xianjingfeng and @advancedxy for the review.

kaijchen requested review from advancedxy and xianjingfeng February 9, 2023 07:33

kaijchen changed the title ~~refactor: replace switch-case with enum and EnumMap in ComposedClientReadHandler~~ refactor: replace switch-case with EnumMap in ComposedClientReadHandler Feb 9, 2023

kaijchen marked this pull request as draft February 9, 2023 08:31

kaijchen added 5 commits February 9, 2023 16:32

refactor: enum Tier in ComposedClientReadHandler

8569dc1

omit value in Tier

2c49e93

replace switch-case by EnumMap

1702a68

improve test

4df67df

add metrics info helper method

33fabd7

kaijchen force-pushed the tier branch from 6d07055 to 33fabd7 Compare February 9, 2023 08:49

kaijchen marked this pull request as ready for review February 9, 2023 08:49

xianjingfeng reviewed Feb 9, 2023

View reviewed changes

storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Feb 9, 2023

View reviewed changes

storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java Outdated Show resolved Hide resolved

add Tier.VALUES

c68384c

advancedxy reviewed Feb 9, 2023

View reviewed changes

kaijchen added 2 commits February 9, 2023 17:18

check argument in constructor

b19ccfc

use StringBuilder

64f8b16

advancedxy reviewed Feb 9, 2023

View reviewed changes

kaijchen added 2 commits February 9, 2023 17:36

throw RssException when handler is null

b79a930

comment initial value

3b725d5

kaijchen changed the title ~~refactor: replace switch-case with EnumMap in ComposedClientReadHandler~~ [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler Feb 9, 2023

kaijchen added 2 commits February 9, 2023 23:18

filter nonNull in close

28da143

init metricsMap in initializer block

ec3be0d

xianjingfeng reviewed Feb 10, 2023

View reviewed changes

xianjingfeng linked an issue Feb 10, 2023 that may be closed by this pull request

[Improvement] Refactor ComposedClientReadHandler #575

Closed

3 tasks

xianjingfeng approved these changes Feb 10, 2023

View reviewed changes

xianjingfeng merged commit 9da9bb6 into apache:master Feb 10, 2023

kaijchen deleted the tier branch February 10, 2023 03:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler #570

[#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler #570

kaijchen commented Feb 9, 2023

codecov-commenter commented Feb 9, 2023 •

edited

xianjingfeng Feb 9, 2023

kaijchen Feb 9, 2023

xianjingfeng Feb 9, 2023

kaijchen Feb 9, 2023

xianjingfeng Feb 9, 2023

advancedxy Feb 9, 2023

advancedxy left a comment

advancedxy Feb 9, 2023

kaijchen Feb 9, 2023

kaijchen Feb 9, 2023

xianjingfeng Feb 10, 2023

kaijchen Feb 10, 2023

xianjingfeng Feb 10, 2023

kaijchen Feb 10, 2023

xianjingfeng Feb 10, 2023

kaijchen Feb 10, 2023

xianjingfeng Feb 10, 2023

kaijchen Feb 10, 2023

kaijchen Feb 10, 2023

xianjingfeng Feb 10, 2023

xianjingfeng left a comment

kaijchen commented Feb 10, 2023

[#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler #570

[#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler #570

Conversation

kaijchen commented Feb 9, 2023

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?

codecov-commenter commented Feb 9, 2023 • edited

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

advancedxy left a 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

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

xianjingfeng left a comment

Choose a reason for hiding this comment

kaijchen commented Feb 10, 2023

codecov-commenter commented Feb 9, 2023 •

edited