You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "kaijchen (via GitHub)" <gi...@apache.org> on 2023/02/09 07:33:48 UTC
[GitHub] [incubator-uniffle] kaijchen opened a new pull request, #570: refactor: replace switch-case with enum and EnumMap in ComposedClientReadHandler
kaijchen opened a new pull request, #570:
URL: https://github.com/apache/incubator-uniffle/pull/570
### 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.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102202377
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
Is `filter(Objects::nonNull)` necessary?
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101157487
##########
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:
Should an exception be thrown here?
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102205005
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
Not required, but just in case.
And to be consistent with 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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102225500
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
There are at most 4 elements in the map ...
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101152101
##########
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);
Review Comment:
If `handlers.length` > `Tier.values().length`, should an exception be thrown?
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102226917
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
It's just for safety.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102240224
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
My mistake. When we get value from `EnumMap`, it will be modified to null again.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102222847
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
I think we can remove it here, because there is an extra loop. WDYT?
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101200109
##########
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];
Review Comment:
I have added a comment here.
--
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
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101175072
##########
integration-test/common/src/test/java/org/apache/uniffle/test/ShuffleServerWithMemLocalHdfsTest.java:
##########
@@ -211,31 +212,31 @@ private void runTest(boolean checkSkippedMetrics) throws Exception {
assertNull(sdr);
if (checkSkippedMetrics) {
- String readBlokNumInfo = composedClientReadHandler.getReadBlokNumInfo();
- 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();
Review Comment:
:+1:
--
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
[GitHub] [incubator-uniffle] xianjingfeng merged pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng merged PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570
--
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
[GitHub] [incubator-uniffle] kaijchen commented on pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#issuecomment-1425136165
Thanks @xianjingfeng and @advancedxy for the review.
--
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
[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101192436
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -190,71 +120,46 @@ public void updateConsumedBlockInfo(BufferSegment bs, boolean isSkippedMetrics)
return;
}
super.updateConsumedBlockInfo(bs, isSkippedMetrics);
- switch (currentHandler) {
- case HOT:
- updateBlockMetric(hostHandlerMetric, bs, isSkippedMetrics);
- break;
- case WARM:
- updateBlockMetric(warmHandlerMetric, bs, isSkippedMetrics);
- break;
- case COLD:
- updateBlockMetric(coldHandlerMetric, bs, isSkippedMetrics);
- break;
- case FROZEN:
- updateBlockMetric(frozenHandlerMetric, bs, isSkippedMetrics);
- break;
- default:
- break;
- }
+ updateBlockMetric(metricsMap.get(currentTier), bs, isSkippedMetrics);
}
@Override
public void logConsumedBlockInfo() {
- LOG.info(getReadBlokNumInfo());
+ LOG.info(getReadBlockNumInfo());
LOG.info(getReadLengthInfo());
LOG.info(getReadUncompressLengthInfo());
}
@VisibleForTesting
- public String getReadBlokNumInfo() {
- return "Client read " + readHandlerMetric.getReadBlockNum()
- + " blocks from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadBlockNum()
- + " warm:" + warmHandlerMetric.getReadBlockNum()
- + " cold:" + coldHandlerMetric.getReadBlockNum()
- + " frozen:" + frozenHandlerMetric.getReadBlockNum()
- + " ], Skipped[" + " hot:" + hostHandlerMetric.getSkippedReadBlockNum()
- + " warm:" + warmHandlerMetric.getSkippedReadBlockNum()
- + " cold:" + coldHandlerMetric.getSkippedReadBlockNum()
- + " frozen:" + frozenHandlerMetric.getSkippedReadBlockNum() + " ]";
+ public String getReadBlockNumInfo() {
+ return getMetricsInfo("blocks", ClientReadHandlerMetric::getReadBlockNum,
+ ClientReadHandlerMetric::getSkippedReadBlockNum);
}
@VisibleForTesting
public String getReadLengthInfo() {
- return "Client read " + readHandlerMetric.getReadLength()
- + " bytes from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadLength()
- + " warm:" + warmHandlerMetric.getReadLength()
- + " cold:" + coldHandlerMetric.getReadLength()
- + " frozen:" + frozenHandlerMetric.getReadLength() + " ], Skipped["
- + " hot:" + hostHandlerMetric.getSkippedReadLength()
- + " warm:" + warmHandlerMetric.getSkippedReadLength()
- + " cold:" + coldHandlerMetric.getSkippedReadLength()
- + " frozen:" + frozenHandlerMetric.getSkippedReadLength() + " ]";
+ return getMetricsInfo("bytes", ClientReadHandlerMetric::getReadLength,
+ ClientReadHandlerMetric::getSkippedReadLength);
}
@VisibleForTesting
public String getReadUncompressLengthInfo() {
- return "Client read " + readHandlerMetric.getReadUncompressLength()
- + " uncompressed bytes from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadUncompressLength()
- + " warm:" + warmHandlerMetric.getReadUncompressLength()
- + " cold:" + coldHandlerMetric.getReadUncompressLength()
- + " frozen:" + frozenHandlerMetric.getReadUncompressLength() + " ], Skipped["
- + " hot:" + hostHandlerMetric.getSkippedReadUncompressLength()
- + " warm:" + warmHandlerMetric.getSkippedReadUncompressLength()
- + " cold:" + coldHandlerMetric.getSkippedReadUncompressLength()
- + " frozen:" + frozenHandlerMetric.getSkippedReadUncompressLength() + " ]";
+ return getMetricsInfo("uncompressed bytes", ClientReadHandlerMetric::getReadUncompressLength,
+ ClientReadHandlerMetric::getSkippedReadUncompressLength);
+ }
+
+ private String getMetricsInfo(String name, Function<ClientReadHandlerMetric, Long> consumed,
+ Function<ClientReadHandlerMetric, Long> skipped) {
+ return "Client read " + consumed.apply(readHandlerMetric)
Review Comment:
👍
##########
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];
Review Comment:
this seems a bit strange. Could we just use `Hot` here?
--
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
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#issuecomment-1423805343
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#570](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6d07055) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/f461460c1e8090082c7f83851fa99f72fec4f4ed?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f461460) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #570 +/- ##
===========================================
Coverage 60.87% 60.88%
+ Complexity 1796 1569 -227
===========================================
Files 213 191 -22
Lines 12375 10471 -1904
Branches 1049 872 -177
===========================================
- Hits 7533 6375 -1158
+ Misses 4435 3732 -703
+ Partials 407 364 -43
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...uniffle/storage/factory/ShuffleHandlerFactory.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2ZhY3RvcnkvU2h1ZmZsZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...torage/handler/impl/ComposedClientReadHandler.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3RvcmFnZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvdW5pZmZsZS9zdG9yYWdlL2hhbmRsZXIvaW1wbC9Db21wb3NlZENsaWVudFJlYWRIYW5kbGVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...ava/org/apache/uniffle/server/ShuffleTaskInfo.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlVGFza0luZm8uamF2YQ==) | `98.07% <0.00%> (-1.93%)` | :arrow_down: |
| [...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL01SSWRIZWxwZXIuamF2YQ==) | | |
| [...org/apache/spark/shuffle/RssSparkShuffleUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya1NodWZmbGVVdGlscy5qYXZh) | | |
| [...rg/apache/hadoop/mapred/RssMapOutputCollector.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkL1Jzc01hcE91dHB1dENvbGxlY3Rvci5qYXZh) | | |
| [.../java/org/apache/hadoop/mapreduce/RssMRConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL1Jzc01SQ29uZmlnLmphdmE=) | | |
| [.../hadoop/mapreduce/task/reduce/RssBypassWriter.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LW1yL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9oYWRvb3AvbWFwcmVkdWNlL3Rhc2svcmVkdWNlL1Jzc0J5cGFzc1dyaXRlci5qYXZh) | | |
| [.../java/org/apache/spark/shuffle/RssSparkConfig.java](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y2xpZW50LXNwYXJrL2NvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc3Bhcmsvc2h1ZmZsZS9Sc3NTcGFya0NvbmZpZy5qYXZh) | | |
| ... and [16 more](https://codecov.io/gh/apache/incubator-uniffle/pull/570?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101165088
##########
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];
Review Comment:
Good point.
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101198352
##########
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];
Review Comment:
Just in case of we add some new `Super Hot` tier before `Hot`.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102226516
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
The key is whether it is necessary to keep it. It's OK for me too to keep it.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101149569
##########
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];
Review Comment:
Should `values()` be saved to a static variable?
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101190034
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -190,71 +120,46 @@ public void updateConsumedBlockInfo(BufferSegment bs, boolean isSkippedMetrics)
return;
}
super.updateConsumedBlockInfo(bs, isSkippedMetrics);
- switch (currentHandler) {
- case HOT:
- updateBlockMetric(hostHandlerMetric, bs, isSkippedMetrics);
- break;
- case WARM:
- updateBlockMetric(warmHandlerMetric, bs, isSkippedMetrics);
- break;
- case COLD:
- updateBlockMetric(coldHandlerMetric, bs, isSkippedMetrics);
- break;
- case FROZEN:
- updateBlockMetric(frozenHandlerMetric, bs, isSkippedMetrics);
- break;
- default:
- break;
- }
+ updateBlockMetric(metricsMap.get(currentTier), bs, isSkippedMetrics);
}
@Override
public void logConsumedBlockInfo() {
- LOG.info(getReadBlokNumInfo());
+ LOG.info(getReadBlockNumInfo());
LOG.info(getReadLengthInfo());
LOG.info(getReadUncompressLengthInfo());
}
@VisibleForTesting
- public String getReadBlokNumInfo() {
- return "Client read " + readHandlerMetric.getReadBlockNum()
- + " blocks from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadBlockNum()
- + " warm:" + warmHandlerMetric.getReadBlockNum()
- + " cold:" + coldHandlerMetric.getReadBlockNum()
- + " frozen:" + frozenHandlerMetric.getReadBlockNum()
- + " ], Skipped[" + " hot:" + hostHandlerMetric.getSkippedReadBlockNum()
- + " warm:" + warmHandlerMetric.getSkippedReadBlockNum()
- + " cold:" + coldHandlerMetric.getSkippedReadBlockNum()
- + " frozen:" + frozenHandlerMetric.getSkippedReadBlockNum() + " ]";
+ public String getReadBlockNumInfo() {
+ return getMetricsInfo("blocks", ClientReadHandlerMetric::getReadBlockNum,
+ ClientReadHandlerMetric::getSkippedReadBlockNum);
}
@VisibleForTesting
public String getReadLengthInfo() {
- return "Client read " + readHandlerMetric.getReadLength()
- + " bytes from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadLength()
- + " warm:" + warmHandlerMetric.getReadLength()
- + " cold:" + coldHandlerMetric.getReadLength()
- + " frozen:" + frozenHandlerMetric.getReadLength() + " ], Skipped["
- + " hot:" + hostHandlerMetric.getSkippedReadLength()
- + " warm:" + warmHandlerMetric.getSkippedReadLength()
- + " cold:" + coldHandlerMetric.getSkippedReadLength()
- + " frozen:" + frozenHandlerMetric.getSkippedReadLength() + " ]";
+ return getMetricsInfo("bytes", ClientReadHandlerMetric::getReadLength,
+ ClientReadHandlerMetric::getSkippedReadLength);
}
@VisibleForTesting
public String getReadUncompressLengthInfo() {
- return "Client read " + readHandlerMetric.getReadUncompressLength()
- + " uncompressed bytes from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadUncompressLength()
- + " warm:" + warmHandlerMetric.getReadUncompressLength()
- + " cold:" + coldHandlerMetric.getReadUncompressLength()
- + " frozen:" + frozenHandlerMetric.getReadUncompressLength() + " ], Skipped["
- + " hot:" + hostHandlerMetric.getSkippedReadUncompressLength()
- + " warm:" + warmHandlerMetric.getSkippedReadUncompressLength()
- + " cold:" + coldHandlerMetric.getSkippedReadUncompressLength()
- + " frozen:" + frozenHandlerMetric.getSkippedReadUncompressLength() + " ]";
+ return getMetricsInfo("uncompressed bytes", ClientReadHandlerMetric::getReadUncompressLength,
+ ClientReadHandlerMetric::getSkippedReadUncompressLength);
+ }
+
+ private String getMetricsInfo(String name, Function<ClientReadHandlerMetric, Long> consumed,
+ Function<ClientReadHandlerMetric, Long> skipped) {
+ return "Client read " + consumed.apply(readHandlerMetric)
Review Comment:
OK. `StringBuilder` is less intuitive though.
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101190989
##########
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);
Review Comment:
Done.
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101165352
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -190,71 +120,46 @@ public void updateConsumedBlockInfo(BufferSegment bs, boolean isSkippedMetrics)
return;
}
super.updateConsumedBlockInfo(bs, isSkippedMetrics);
- switch (currentHandler) {
- case HOT:
- updateBlockMetric(hostHandlerMetric, bs, isSkippedMetrics);
- break;
- case WARM:
- updateBlockMetric(warmHandlerMetric, bs, isSkippedMetrics);
- break;
- case COLD:
- updateBlockMetric(coldHandlerMetric, bs, isSkippedMetrics);
- break;
- case FROZEN:
- updateBlockMetric(frozenHandlerMetric, bs, isSkippedMetrics);
- break;
- default:
- break;
- }
+ updateBlockMetric(metricsMap.get(currentTier), bs, isSkippedMetrics);
}
@Override
public void logConsumedBlockInfo() {
- LOG.info(getReadBlokNumInfo());
+ LOG.info(getReadBlockNumInfo());
LOG.info(getReadLengthInfo());
LOG.info(getReadUncompressLengthInfo());
}
@VisibleForTesting
- public String getReadBlokNumInfo() {
- return "Client read " + readHandlerMetric.getReadBlockNum()
- + " blocks from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadBlockNum()
- + " warm:" + warmHandlerMetric.getReadBlockNum()
- + " cold:" + coldHandlerMetric.getReadBlockNum()
- + " frozen:" + frozenHandlerMetric.getReadBlockNum()
- + " ], Skipped[" + " hot:" + hostHandlerMetric.getSkippedReadBlockNum()
- + " warm:" + warmHandlerMetric.getSkippedReadBlockNum()
- + " cold:" + coldHandlerMetric.getSkippedReadBlockNum()
- + " frozen:" + frozenHandlerMetric.getSkippedReadBlockNum() + " ]";
+ public String getReadBlockNumInfo() {
+ return getMetricsInfo("blocks", ClientReadHandlerMetric::getReadBlockNum,
+ ClientReadHandlerMetric::getSkippedReadBlockNum);
}
@VisibleForTesting
public String getReadLengthInfo() {
- return "Client read " + readHandlerMetric.getReadLength()
- + " bytes from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadLength()
- + " warm:" + warmHandlerMetric.getReadLength()
- + " cold:" + coldHandlerMetric.getReadLength()
- + " frozen:" + frozenHandlerMetric.getReadLength() + " ], Skipped["
- + " hot:" + hostHandlerMetric.getSkippedReadLength()
- + " warm:" + warmHandlerMetric.getSkippedReadLength()
- + " cold:" + coldHandlerMetric.getSkippedReadLength()
- + " frozen:" + frozenHandlerMetric.getSkippedReadLength() + " ]";
+ return getMetricsInfo("bytes", ClientReadHandlerMetric::getReadLength,
+ ClientReadHandlerMetric::getSkippedReadLength);
}
@VisibleForTesting
public String getReadUncompressLengthInfo() {
- return "Client read " + readHandlerMetric.getReadUncompressLength()
- + " uncompressed bytes from [" + serverInfo + "], Consumed["
- + " hot:" + hostHandlerMetric.getReadUncompressLength()
- + " warm:" + warmHandlerMetric.getReadUncompressLength()
- + " cold:" + coldHandlerMetric.getReadUncompressLength()
- + " frozen:" + frozenHandlerMetric.getReadUncompressLength() + " ], Skipped["
- + " hot:" + hostHandlerMetric.getSkippedReadUncompressLength()
- + " warm:" + warmHandlerMetric.getSkippedReadUncompressLength()
- + " cold:" + coldHandlerMetric.getSkippedReadUncompressLength()
- + " frozen:" + frozenHandlerMetric.getSkippedReadUncompressLength() + " ]";
+ return getMetricsInfo("uncompressed bytes", ClientReadHandlerMetric::getReadUncompressLength,
+ ClientReadHandlerMetric::getSkippedReadUncompressLength);
+ }
+
+ private String getMetricsInfo(String name, Function<ClientReadHandlerMetric, Long> consumed,
+ Function<ClientReadHandlerMetric, Long> skipped) {
+ return "Client read " + consumed.apply(readHandlerMetric)
Review Comment:
Could you change it to StringBuilder by the way?
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1101174431
##########
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:
Yes, but it is unreasonable. It means there is a bug if `handler=null`. It may increase the difficulty of discover problems.
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102235373
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
> 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.
<img width="663" alt="test" src="https://user-images.githubusercontent.com/5821159/217992019-510a2a52-8688-4d8e-87d5-3cac1b6f0845.png">
--
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
[GitHub] [incubator-uniffle] kaijchen commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "kaijchen (via GitHub)" <gi...@apache.org>.
kaijchen commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102232615
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
OK, let me check.
--
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
[GitHub] [incubator-uniffle] xianjingfeng commented on a diff in pull request #570: [#575] refactor: replace switch-case with EnumMap in ComposedClientReadHandler
Posted by "xianjingfeng (via GitHub)" <gi...@apache.org>.
xianjingfeng commented on code in PR #570:
URL: https://github.com/apache/incubator-uniffle/pull/570#discussion_r1102231477
##########
storage/src/main/java/org/apache/uniffle/storage/handler/impl/ComposedClientReadHandler.java:
##########
@@ -144,44 +115,9 @@ public ShuffleDataResult readShuffleData() {
return shuffleDataResult;
}
- private String getCurrentHandlerName() {
- String name = "UNKNOWN";
- switch (currentHandler) {
- case HOT:
- name = "HOT";
- break;
- case WARM:
- name = "WARM";
- break;
- case COLD:
- name = "COLD";
- break;
- case FROZEN:
- name = "FROZEN";
- break;
- default:
- break;
- }
- return name;
- }
-
@Override
public void close() {
- if (hotDataReadHandler != null) {
- hotDataReadHandler.close();
- }
-
- if (warmDataReadHandler != null) {
- warmDataReadHandler.close();
- }
-
- if (coldDataReadHandler != null) {
- coldDataReadHandler.close();
- }
-
- if (frozenDataReadHandler != null) {
- frozenDataReadHandler.close();
- }
+ handlerMap.values().stream().filter(Objects::nonNull).forEach(ClientReadHandler::close);
Review Comment:
Actually, the values of `EnumMap` will not be null. If we put `null` to `EnumMap`, it will be modified to an instance of `Object`.
--
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