[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler

["kaijchen (via GitHub)" <gi...@apache.org>]

kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101164067


##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -40,101 +43,61 @@ public class ComposedClientReadHandler extends AbstractClientReadHandler {
 
   private static final Logger LOG = LoggerFactory.getLogger(ComposedClientReadHandler.class);
 
-  private final ShuffleServerInfo serverInfo;
-  private Callable<ClientReadHandler> hotHandlerCreator;
-  private Callable<ClientReadHandler> warmHandlerCreator;
-  private Callable<ClientReadHandler> coldHandlerCreator;
-  private Callable<ClientReadHandler> frozenHandlerCreator;
-  private ClientReadHandler hotDataReadHandler;
-  private ClientReadHandler warmDataReadHandler;
-  private ClientReadHandler coldDataReadHandler;
-  private ClientReadHandler frozenDataReadHandler;
-  private static final int HOT = 1;
-  private static final int WARM = 2;
-  private static final int COLD = 3;
-  private static final int FROZEN = 4;
-  private int currentHandler = HOT;
-  private final int topLevelOfHandler;
+  private enum Tier {
+    HOT, WARM, COLD, FROZEN;
+
+    public Tier next() {
+      return values()[this.ordinal() + 1];
+    }
+  }
 
-  private ClientReadHandlerMetric hostHandlerMetric = new ClientReadHandlerMetric();
-  private ClientReadHandlerMetric warmHandlerMetric = new ClientReadHandlerMetric();
-  private ClientReadHandlerMetric coldHandlerMetric = new ClientReadHandlerMetric();
-  private ClientReadHandlerMetric frozenHandlerMetric = new ClientReadHandlerMetric();
+  private final ShuffleServerInfo serverInfo;
+  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];
+  private final int numTiers;
 
   public ComposedClientReadHandler(ShuffleServerInfo serverInfo, ClientReadHandler... handlers) {
     this.serverInfo = serverInfo;
-    topLevelOfHandler = handlers.length;
-    if (topLevelOfHandler > 0) {
-      this.hotDataReadHandler = handlers[0];
-    }
-    if (topLevelOfHandler > 1) {
-      this.warmDataReadHandler = handlers[1];
+    numTiers = Math.min(Tier.values().length, handlers.length);
+    for (int i = 0; i < numTiers; i++) {
+      handlerMap.put(Tier.values()[i], handlers[i]);
     }
-    if (topLevelOfHandler > 2) {
-      this.coldDataReadHandler = handlers[2];
-    }
-    if (topLevelOfHandler > 3) {
-      this.frozenDataReadHandler = handlers[3];
+    for (Tier tier : Tier.values()) {
+      metricsMap.put(tier, new ClientReadHandlerMetric());
     }
   }
 
-  public ComposedClientReadHandler(ShuffleServerInfo serverInfo, List<Callable<ClientReadHandler>> callables) {
+  public ComposedClientReadHandler(ShuffleServerInfo serverInfo, List<Supplier<ClientReadHandler>> callables) {
     this.serverInfo = serverInfo;
-    topLevelOfHandler = callables.size();
-    if (topLevelOfHandler > 0) {
-      this.hotHandlerCreator = callables.get(0);
-    }
-    if (topLevelOfHandler > 1) {
-      this.warmHandlerCreator = callables.get(1);
-    }
-    if (topLevelOfHandler > 2) {
-      this.coldHandlerCreator = callables.get(2);
+    numTiers = Math.min(Tier.values().length, callables.size());
+    for (int i = 0; i < numTiers; i++) {
+      supplierMap.put(Tier.values()[i], callables.get(i));
     }
-    if (topLevelOfHandler > 3) {
-      this.frozenHandlerCreator = callables.get(3);
+    for (Tier tier : Tier.values()) {
+      metricsMap.put(tier, new ClientReadHandlerMetric());
     }
   }
 
   @Override
   public ShuffleDataResult readShuffleData() {
     ShuffleDataResult shuffleDataResult = null;
     try {
-      switch (currentHandler) {
-        case HOT:
-          if (hotDataReadHandler == null) {
-            hotDataReadHandler = hotHandlerCreator.call();
-          }
-          shuffleDataResult = hotDataReadHandler.readShuffleData();
-          break;
-        case WARM:
-          if (warmDataReadHandler == null) {
-            warmDataReadHandler = warmHandlerCreator.call();
-          }
-          shuffleDataResult = warmDataReadHandler.readShuffleData();
-          break;
-        case COLD:
-          if (coldDataReadHandler == null) {
-            coldDataReadHandler = coldHandlerCreator.call();
-          }
-          shuffleDataResult = coldDataReadHandler.readShuffleData();
-          break;
-        case FROZEN:
-          if (frozenDataReadHandler == null) {
-            frozenDataReadHandler = frozenHandlerCreator.call();
-          }
-          shuffleDataResult = frozenDataReadHandler.readShuffleData();
-          break;
-        default:
-          return null;
+      ClientReadHandler handler = handlerMap.computeIfAbsent(currentTier,
+          key -> supplierMap.getOrDefault(key, () -> null).get());
+      if (handler == null) {
+        return null;

Review Comment:
   The current logic is equivalent to the old logic.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org