[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 (via GitHub)" <gi...@apache.org>]

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 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.



-- 
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