You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sanpwc (via GitHub)" <gi...@apache.org> on 2023/04/28 07:50:42 UTC
[GitHub] [ignite-3] sanpwc commented on a diff in pull request #1968: IGNITE-18624 Data nodes from DistributionZoneManager instead of BaselineManager#nodes on table creation.
sanpwc commented on code in PR #1968:
URL: https://github.com/apache/ignite-3/pull/1968#discussion_r1180034904
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -161,6 +171,17 @@ public class DistributionZoneManager implements IgniteComponent {
/** The logger. */
private static final IgniteLogger LOG = Loggers.forClass(DistributionZoneManager.class);
+ /**
+ * If this property is set to {@code true} then an attempt to get the configuration property directly from Meta storage will be skipped,
+ * and the local property will be returned.
+ * TODO: IGNITE-16774 This property and overall approach, access configuration directly through Meta storage,
+ * TODO: will be removed after fix of the issue.
+ */
+ private final boolean getMetadataLocallyOnly = IgniteSystemProperties.getBoolean("IGNITE_GET_METADATA_LOCALLY_ONLY");
+
+ /** Versioned store for zones id. */
+ private final IncrementalVersionedValue<Set<Integer>> zonesByIdVv;
Review Comment:
I don't think that you need versioned value here because you only use the it's side effects and nor it's core idea.
E.g. it's possible to add cfg listener in order to await zone with needed id. I mean following:
1. Read zone cfg through direct proxy and check whether zone with given name exists. Throw an exception if it doesn't.
2. Register zones listener in order to await zone locally with given Id.
3. Check whether we already have such zone locally. Remove listener if we do have one.
2' Within listener we should do some useful stuff and remove the listener itself. Please ask SE guys whether it's possible to unregister the listener from within listener.
```
DistributionZoneConfiguration zoneCfg = directProxy(zonesConfiguration.distributionZones()).get(zoneName);
// check that zone is present.
zonesConfiguration.distributionZones().listenElements(new ConfigurationNamedListListener<DistributionZoneView>() {
@Override
public CompletableFuture<?> onCreate(ConfigurationNotificationEvent<DistributionZoneView> ctx) {
if (ctx.newValue().zoneId() == zoneCfg.zoneId().value()) {
// do something usefull
// unregister current listener. Please ask SE guys whether it's possible to unregister the listener from within listener.
}
return CompletableFuture.completedFuture(null);
}
});
```
I'm not sure whether solution above is best, however it seems better to versioned value one. We can think about other options too.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -859,7 +885,17 @@ public CompletableFuture<?> onCreate(ConfigurationNotificationEvent<Distribution
saveDataNodesAndUpdateTriggerKeysInMetaStorage(zoneId, ctx.storageRevision(), logicalTopology);
- return completedFuture(null);
+ return zonesByIdVv.update(ctx.storageRevision(), (zones, e) -> {
+ if (e != null) {
Review Comment:
What about busy lock? Do we need it here?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -1614,6 +1662,101 @@ CompletableFuture<Void> saveDataNodesToMetaStorageOnScaleDown(int zoneId, long r
}
}
+ /**
+ * Gets direct id of the distribution zone with {@code zoneName}.
+ *
+ * @param zoneName Name of the distribution zone.
+ * @return Direct id of the distribution zone, or {@code null} if the zone with the {@code zoneName} has not been found.
+ */
+ public CompletableFuture<Integer> zoneIdAsyncInternal(String zoneName) {
+ if (!busyLock.enterBusy()) {
+ throw new IgniteException(new NodeStoppingException());
+ }
+ try {
+ if (DEFAULT_ZONE_NAME.equals(zoneName)) {
+ return completedFuture(DEFAULT_ZONE_ID);
+ }
+
+ // TODO: IGNITE-16288 directZoneId should use async configuration API
+ return supplyAsync(() -> inBusyLock(busyLock, () -> directZoneIdInternal(zoneName)), executor)
+ .thenCompose(zoneId -> waitZoneIdLocally(zoneId).thenCompose(ignored -> completedFuture(zoneId)));
+ } finally {
+ busyLock.leaveBusy();
+ }
+ }
+
+ @Nullable
+ private Integer directZoneIdInternal(String zoneName) {
+ try {
+ DistributionZoneConfiguration zoneCfg = directProxy(zonesConfiguration.distributionZones()).get(zoneName);
+
+ if (zoneCfg == null) {
+ return null;
+ } else {
+ return zoneCfg.zoneId().value();
+ }
+ } catch (NoSuchElementException e) {
+ return null;
+ }
+ }
+
+ /**
+ * Internal method for waiting that the zone is created locally.
+ *
+ * @param id Table id.
+ * @return Future representing pending completion of the operation.
+ */
+ private CompletableFuture<Void> waitZoneIdLocally(Integer id) {
Review Comment:
I'm not sure that I get how you handle null as an id. I mean that directZoneIdInternal may return null if there's no such zone in direct cfg.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -1614,6 +1662,101 @@ CompletableFuture<Void> saveDataNodesToMetaStorageOnScaleDown(int zoneId, long r
}
}
+ /**
+ * Gets direct id of the distribution zone with {@code zoneName}.
+ *
+ * @param zoneName Name of the distribution zone.
+ * @return Direct id of the distribution zone, or {@code null} if the zone with the {@code zoneName} has not been found.
+ */
+ public CompletableFuture<Integer> zoneIdAsyncInternal(String zoneName) {
+ if (!busyLock.enterBusy()) {
+ throw new IgniteException(new NodeStoppingException());
+ }
+ try {
+ if (DEFAULT_ZONE_NAME.equals(zoneName)) {
+ return completedFuture(DEFAULT_ZONE_ID);
+ }
+
+ // TODO: IGNITE-16288 directZoneId should use async configuration API
+ return supplyAsync(() -> inBusyLock(busyLock, () -> directZoneIdInternal(zoneName)), executor)
+ .thenCompose(zoneId -> waitZoneIdLocally(zoneId).thenCompose(ignored -> completedFuture(zoneId)));
+ } finally {
+ busyLock.leaveBusy();
+ }
+ }
+
+ @Nullable
+ private Integer directZoneIdInternal(String zoneName) {
+ try {
+ DistributionZoneConfiguration zoneCfg = directProxy(zonesConfiguration.distributionZones()).get(zoneName);
+
+ if (zoneCfg == null) {
+ return null;
+ } else {
+ return zoneCfg.zoneId().value();
+ }
+ } catch (NoSuchElementException e) {
+ return null;
+ }
+ }
+
+ /**
+ * Internal method for waiting that the zone is created locally.
+ *
+ * @param id Table id.
Review Comment:
Seems that it's not table but zone id.
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DistributionZoneManager.java:
##########
@@ -859,7 +885,17 @@ public CompletableFuture<?> onCreate(ConfigurationNotificationEvent<Distribution
saveDataNodesAndUpdateTriggerKeysInMetaStorage(zoneId, ctx.storageRevision(), logicalTopology);
- return completedFuture(null);
+ return zonesByIdVv.update(ctx.storageRevision(), (zones, e) -> {
+ if (e != null) {
+ return failedFuture(e);
+ }
+
+ HashSet<Integer> newZones = new HashSet<>(zones);
Review Comment:
Why do we need to create a new set on every addition/removal?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org