sanpwc commented on code in PR #1426:
URL: https://github.com/apache/ignite-3/pull/1426#discussion_r1057492859
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -108,24 +120,32 @@ public void onTopologyLeap(LogicalTopologySnapshot
newTopology) {
}
};
+ /** The logical topology on the last watch event. */
+ private volatile Set<String> logicalTopology;
Review Comment:
Please add comment that will explain why volatile is enough here.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -192,7 +217,7 @@ public CompletableFuture<Void> alterZone(String name,
DistributionZoneConfigurat
Objects.requireNonNull(distributionZoneCfg, "Distribution zone
configuration is null.");
if (!busyLock.enterBusy()) {
- throw new IgniteException(new NodeStoppingException());
+ throw new IgniteException(NODE_STOPPING_ERR, new
NodeStoppingException());
Review Comment:
What's the point of given code?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -275,7 +310,9 @@ public void start() {
logicalTopologyService.addEventListener(topologyEventListener);
- initMetaStorageKeysOnStart();
+ registerMetaStorageWatchListener()
Review Comment:
What will happen in case of concurrent start/stop calls?
Busy locks are missing almost everywhere.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -204,7 +229,12 @@ public CompletableFuture<Void> alterZone(String name,
DistributionZoneConfigurat
} catch (IllegalArgumentException e) {
throw new DistributionZoneRenameException(name,
distributionZoneCfg.name(), e);
} catch (Exception e) {
- throw new IgniteInternalException(UNEXPECTED_ERR,
distributionZoneCfg.name(), e);
+ throw withCause(
Review Comment:
Same as above.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -137,7 +156,7 @@ public CompletableFuture<Void>
createZone(DistributionZoneConfigurationParameter
Objects.requireNonNull(distributionZoneCfg, "Distribution zone
configuration is null.");
if (!busyLock.enterBusy()) {
- throw new IgniteException(new NodeStoppingException());
+ throw new IgniteException(NODE_STOPPING_ERR, new
NodeStoppingException());
Review Comment:
How did you fix it?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -172,7 +192,12 @@ public CompletableFuture<Void>
createZone(DistributionZoneConfigurationParameter
} catch (IllegalArgumentException e) {
throw new
DistributionZoneAlreadyExistsException(distributionZoneCfg.name(), e);
} catch (Exception e) {
- throw new IgniteInternalException(UNEXPECTED_ERR,
distributionZoneCfg.name(), e);
+ throw withCause(
Review Comment:
Please add all possible exceptions to the @throws clause explaining why it's
happening.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -172,7 +192,12 @@ public CompletableFuture<Void>
createZone(DistributionZoneConfigurationParameter
} catch (IllegalArgumentException e) {
throw new
DistributionZoneAlreadyExistsException(distributionZoneCfg.name(), e);
} catch (Exception e) {
- throw new IgniteInternalException(UNEXPECTED_ERR,
distributionZoneCfg.name(), e);
+ throw withCause(
Review Comment:
What's the point in catching an exception and re-throwing
IgniteInternalException 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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
