You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/08/12 11:03:20 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
tkalkirill opened a new pull request #273:
URL: https://github.com/apache/ignite-3/pull/273
…gurationRegistry
--
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
[GitHub] [ignite-3] ibessonov merged pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
ibessonov merged pull request #273:
URL: https://github.com/apache/ignite-3/pull/273
--
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
[GitHub] [ignite-3] tkalkirill commented on pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on pull request #273:
URL: https://github.com/apache/ignite-3/pull/273#issuecomment-899271948
@ibessonov Please make code 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on pull request #273:
URL: https://github.com/apache/ignite-3/pull/273#issuecomment-898401956
@ibessonov Please make code 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on pull request #273:
URL: https://github.com/apache/ignite-3/pull/273#issuecomment-899318602
@ibessonov Please make code 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #273:
URL: https://github.com/apache/ignite-3/pull/273#discussion_r689293965
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -255,75 +227,36 @@ public void initialize(ConfigurationType storageType) throws ConfigurationValida
throw (ConfigurationChangeException)cause;
throw new ConfigurationChangeException(
- "Failed to write defalut configuration values into the storage " + configurationStorage.getClass(), e
+ "Failed to write default configuration values into the storage " + storage.getClass(), e
);
}
catch (InterruptedException e) {
throw new ConfigurationChangeException(
- "Failed to initialize configuration storage " + configurationStorage.getClass(), e
+ "Failed to initialize configuration storage " + storage.getClass(), e
);
}
}
/**
* Changes the configuration.
+ *
* @param source Configuration source to create patch from.
- * @param storage Expected storage for the changes. If null, a derived storage will be used
- * unconditionaly.
* @return Future that is completed on change completion.
*/
- public CompletableFuture<Void> change(
- ConfigurationSource source,
- @Nullable ConfigurationStorage storage
- ) {
- Set<ConfigurationType> storagesTypes = new HashSet<>();
-
- ConstructableTreeNode collector = new ConstructableTreeNode() {
- @Override public void construct(String key, ConfigurationSource src) throws NoSuchElementException {
- RootKey<?, ?> rootKey = rootKeys.get(key);
-
- if (rootKey == null)
- throw new NoSuchElementException(key);
-
- storagesTypes.add(rootKey.type());
- }
-
- @Override public ConstructableTreeNode copy() {
- throw new UnsupportedOperationException("copy");
- }
- };
-
+ public CompletableFuture<Void> change(ConfigurationSource source) {
source.reset();
Review comment:
This call can be removed as well
##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/SchemaConfigurationConverterTest.java
##########
@@ -68,9 +71,10 @@
@BeforeEach
public void createRegistry() throws ExecutionException, InterruptedException {
confRegistry = new ConfigurationRegistry(
- Collections.singleton(TablesConfiguration.KEY),
- Collections.singletonMap(TableValidator.class, Collections.singleton(SchemaTableValidatorImpl.INSTANCE)),
- Collections.singleton(new TestConfigurationStorage(ConfigurationType.DISTRIBUTED)));
+ List.of(TablesConfiguration.KEY),
+ Map.of(TableValidator.class, Set.of(SchemaTableValidatorImpl.INSTANCE)),
Review comment:
I meant "SchemaTableValidatorImpl.INSTANCE"
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -187,20 +176,27 @@ public void startStorageConfigurations(ConfigurationType storageType) {
/**
* Change configuration.
- * @param changesSource Configuration source to create patch from it.
- * @param storage Expected storage for the changes. Can be null, this will mean that derived storage will be used
- * unconditionaly.
+ *
+ * @param changesSrc Configuration source to create patch from it.
* @return Future that is completed on change completion.
*/
- public CompletableFuture<Void> change(ConfigurationSource changesSource, @Nullable ConfigurationStorage storage) {
- return changer.change(changesSource, storage);
+ public CompletableFuture<Void> change(ConfigurationSource changesSrc) {
+ return changer.change(changesSrc);
}
- /** */
- private @NotNull CompletableFuture<Void> notificator(SuperRoot oldSuperRoot, SuperRoot newSuperRoot, long storageRevision) {
+ /**
+ * Configuration change notifier.
+ *
+ * @param oldSuperRoot Old roots values. All these roots always belong to a single storage.
+ * @param newSuperRoot New values for the same roots as in {@code oldRoot}.
+ * @param storageRevision Revision of the storage.
+ * @return Future that must signify when processing is completed. Exceptional completion is not expected.
Review comment:
You know what, I completely forgot about this... We should fail this future if we encounter any exception in "futures" list inside of this method. Can you please fix 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #273:
URL: https://github.com/apache/ignite-3/pull/273#discussion_r688459605
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/spec/ConfigCommandSpec.java
##########
@@ -81,10 +94,28 @@
@CommandLine.Mixin
private CfgHostnameOptions cfgHostnameOptions;
+ /**
+ * Configuration type: {@code node} or {@code cluster}.
+ *
+ * TODO: Fix in https://issues.apache.org/jira/browse/IGNITE-15285
Review comment:
Same here
##########
File path: modules/cli/src/integrationTest/java/org/apache/ignite/cli/ConfigCommandTest.java
##########
@@ -85,6 +85,7 @@ public void setAndGetWithManualHost() {
"set",
"--node-endpoint",
"localhost:" + restPort,
+ "--type", "node", //TODO: Fix in https://issues.apache.org/jira/browse/IGNITE-15285
Review comment:
Please change the link here and in other places
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -210,36 +193,27 @@ public void register(ConfigurationStorage configurationStorage) throws Configura
superRoot.addRoot(rootKey, rootNode);
}
- StorageRoots storageRoots = new StorageRoots(superRoot, data.changeId());
+ storageRoots = new StorageRoots(superRoot, data.changeId());
- storagesRootsMap.put(configurationStorage.type(), storageRoots);
-
- configurationStorage.registerConfigurationListener(changedEntries -> updateFromListener(
- configurationStorage.type(),
- changedEntries
- ));
+ storage.registerConfigurationListener(this::updateFromListener);
}
/**
* Initializes the configuration storage - reads data and sets default values for missing configuration properties.
- * @param storageType Storage type.
+ *
* @throws ConfigurationValidationException If configuration validation failed.
- * @throws ConfigurationChangeException If configuration framework failed to add default values and save them to
- * storage.
+ * @throws ConfigurationChangeException If configuration framework failed to add default values and save them to storage.
*/
- public void initialize(ConfigurationType storageType) throws ConfigurationValidationException, ConfigurationChangeException {
- ConfigurationStorage configurationStorage = storageInstances.get(storageType);
-
- assert configurationStorage != null : storageType;
-
+ public void initializeDefaults() throws ConfigurationValidationException, ConfigurationChangeException {
try {
- ConfigurationSource defaultsCfgSource = new ConfigurationSource() {
+ ConfigurationSource defaultsCfgSrc = new ConfigurationSource() {
Review comment:
I believe you're using abbreviation plugin. Can we use "source" as a full word if it's a part of the name?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -100,73 +105,58 @@
private final SuperRoot roots;
/** Version associated with the currently known storage state. */
- private final long version;
+ private final long ver;
- /** */
- private StorageRoots(SuperRoot roots, long version) {
+ /**
+ * Constructor.
+ *
+ * @param roots Forest.
+ * @param ver Version associated with the currently known storage state.
+ */
+ private StorageRoots(SuperRoot roots, long ver) {
Review comment:
Same here, we don't have to use short names
##########
File path: modules/rest/src/main/java/org/apache/ignite/rest/RestModule.java
##########
@@ -53,90 +55,88 @@
* Refer to default config file in resources for the example.
*/
public class RestModule implements IgniteComponent {
- /** */
+ /** Default port. */
public static final int DFLT_PORT = 10300;
- /** */
- private static final String CONF_URL = "/management/v1/configuration/";
+ /** Node configuration route. */
+ private static final String NODE_CFG_URL = "/management/v1/configuration/node/";
- /** */
+ /** Cluster configuration route. */
+ private static final String CLUSTER_CFG_URL = "/management/v1/configuration/cluster/";
+
+ /** Path parameter. */
private static final String PATH_PARAM = "selector";
/** Ignite logger. */
private final IgniteLogger LOG = IgniteLogger.forClass(RestModule.class);
- /** */
- private final ConfigurationRegistry sysConf;
+ /** Node configuration register. */
+ private final ConfigurationRegistry nodeCfgRegistry;
+
+ /** Presentation of node configuration. */
+ private final ConfigurationPresentation<String> nodeCfgPresentation;
- /** */
- private volatile ConfigurationPresentation<String> presentation;
+ /** Presentation of cluster configuration. */
+ private final ConfigurationPresentation<String> clusterCfgPresentation;
/**
* Creates a new instance of REST module.
*
* @param nodeCfgMgr Node configuration manager.
+ * @param clusterCfgMgr Cluster configuration manager.
*/
- public RestModule(ConfigurationManager nodeCfgMgr) {
- sysConf = nodeCfgMgr.configurationRegistry();
+ public RestModule(
+ ConfigurationManager nodeCfgMgr,
+ ConfigurationManager clusterCfgMgr
+ ) {
+ nodeCfgRegistry = nodeCfgMgr.registry();
+
+ nodeCfgPresentation = new HoconPresentation(nodeCfgMgr.registry());
+ clusterCfgPresentation = new HoconPresentation(clusterCfgMgr.registry());
}
/** {@inheritDoc} */
@Override public void start() {
- presentation = new HoconPresentation(sysConf);
-
var router = new Router();
router
- .get(CONF_URL, (req, resp) -> {
- resp.json(presentation.represent());
- })
- .get(CONF_URL + ":" + PATH_PARAM, (req, resp) -> {
- String cfgPath = req.queryParams().get(PATH_PARAM);
- try {
- resp.json(presentation.representByPath(cfgPath));
- }
- catch (IllegalArgumentException pathE) {
- ErrorResult eRes = new ErrorResult("CONFIG_PATH_UNRECOGNIZED", pathE.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- })
- .put(CONF_URL, HttpHeaderValues.APPLICATION_JSON, (req, resp) -> {
- try {
- presentation.update(
- req
- .request()
- .content()
- .readCharSequence(req.request().content().readableBytes(), StandardCharsets.UTF_8)
- .toString());
- }
- catch (IllegalArgumentException e) {
- ErrorResult eRes = new ErrorResult("INVALID_CONFIG_FORMAT", e.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- catch (ConfigurationValidationException e) {
- ErrorResult eRes = new ErrorResult("VALIDATION_EXCEPTION", e.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- catch (IgniteException e) {
- ErrorResult eRes = new ErrorResult("APPLICATION_EXCEPTION", e.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- });
+ .get(
+ NODE_CFG_URL,
+ (req, resp) -> resp.json(nodeCfgPresentation.represent())
+ )
+ .get(
+ CLUSTER_CFG_URL,
+ (req, resp) -> resp.json(clusterCfgPresentation.represent())
+ )
+ .get(
+ NODE_CFG_URL + ":" + PATH_PARAM,
+ (req, resp) -> handleRepresentByPath(req, resp, nodeCfgPresentation)
+ )
+ .get(
+ CLUSTER_CFG_URL + ":" + PATH_PARAM,
+ (req, resp) -> handleRepresentByPath(req, resp, clusterCfgPresentation)
+ )
+ .put(
+ NODE_CFG_URL,
+ APPLICATION_JSON,
+ (req, resp) -> handleUpdate(req, resp, nodeCfgPresentation)
+ )
+ .put(
+ CLUSTER_CFG_URL,
+ APPLICATION_JSON,
+ (req, resp) -> handleUpdate(req, resp, clusterCfgPresentation)
+ );
startRestEndpoint(router);
}
- /** */
- private ChannelFuture startRestEndpoint(Router router) {
- RestView restConfigurationView = sysConf.getConfiguration(RestConfiguration.KEY).value();
+ /**
+ * Start endpoint.
+ *
+ * @param router Dispatcher of http requests.
+ */
+ private void startRestEndpoint(Router router) {
Review comment:
What's the deal with changing return type of the method?
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/TestConfigurationChanger.java
##########
@@ -28,20 +35,40 @@
/** Runtime implementations generator for node classes. */
private final ConfigurationAsmGenerator cgen;
+ /** Root keys. */
+ private final Collection<RootKey<?, ?>> rootKeys;
+
/**
+ * Constructor.
+ *
* @param cgen Runtime implementations generator for node classes. Will be used to instantiate nodes objects.
+ * @param rootKeys Configuration root keys.
+ * @param validators Validators.
+ * @param storage Configuration storage.
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
*/
- public TestConfigurationChanger(ConfigurationAsmGenerator cgen) {
- super((oldRoot, newRoot, revision) -> completedFuture(null));
+ public TestConfigurationChanger(
+ ConfigurationAsmGenerator cgen,
+ Collection<RootKey<?, ?>> rootKeys,
+ Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators,
+ ConfigurationStorage storage
+ ) {
+ super(
+ (oldRoot, newRoot, revision) -> completedFuture(null),
+ rootKeys,
+ validators,
+ storage
+ );
this.cgen = cgen;
+ this.rootKeys = rootKeys;
}
/** {@inheritDoc} */
- @Override public void addRootKey(RootKey<?, ?> rootKey) {
- super.addRootKey(rootKey);
+ @Override public void start() throws ConfigurationChangeException {
+ rootKeys.forEach(key -> cgen.compileRootSchema(key.schemaClass()));
Review comment:
You can do this in constructor and remove new field
##########
File path: modules/cli/src/main/java/org/apache/ignite/cli/builtins/config/ConfigurationClient.java
##########
@@ -69,21 +69,24 @@ public ConfigurationClient(HttpClient httpClient) {
* @param host String representation of server node host.
* @param port Host REST port.
* @param rawHoconPath HOCON dot-delimited path of requested configuration.
+ * @param type Configuration type: {@code node} or {@code cluster}.
+ * TODO: Fix in https://issues.apache.org/jira/browse/IGNITE-15285
Review comment:
It this a valid TODO syntax? Can IDEA recognize it? I would suggest creating additional single-line comment instead
##########
File path: modules/cli/src/test/java/org/apache/ignite/cli/IgniteCliInterfaceTest.java
##########
@@ -508,22 +508,22 @@ void get() throws IOException, InterruptedException {
"}\n", out.toString());
}
- /** */
+ /** //TODO: Fix in https://issues.apache.org/jira/browse/IGNITE-15285 */
Review comment:
What is this syntax?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -100,73 +105,58 @@
private final SuperRoot roots;
/** Version associated with the currently known storage state. */
- private final long version;
+ private final long ver;
Review comment:
What's the reason for shortening the name? If anything, you should have renamed it to "changeId" :)
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -76,17 +76,13 @@
/** */
@BeforeEach
public void before() {
- var testConfigurationStorage = new TestConfigurationStorage(ConfigurationType.LOCAL);
+ var testConfigurationStorage = new TestConfigurationStorage(LOCAL);
- registry = new ConfigurationRegistry(
- Collections.singletonList(ParentConfiguration.KEY),
- Collections.emptyMap(),
- Collections.singletonList(testConfigurationStorage)
- );
+ registry = new ConfigurationRegistry(List.of(ParentConfiguration.KEY), Map.of(), testConfigurationStorage);
registry.start();
- registry.startStorageConfigurations(testConfigurationStorage.type());
+ registry.initializeDefaultsStorage();
Review comment:
Word "Storage" seems out of place
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -100,73 +105,58 @@
private final SuperRoot roots;
/** Version associated with the currently known storage state. */
- private final long version;
+ private final long ver;
- /** */
- private StorageRoots(SuperRoot roots, long version) {
+ /**
+ * Constructor.
+ *
+ * @param roots Forest.
+ * @param ver Version associated with the currently known storage state.
+ */
+ private StorageRoots(SuperRoot roots, long ver) {
this.roots = roots;
- this.version = version;
+ this.ver = ver;
}
}
- /** Lazy annotations cache for configuration schema fields. */
- private final Map<MemberKey, Annotation[]> cachedAnnotations = new ConcurrentHashMap<>();
-
- /** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
- private final Map<ConfigurationType, ConfigurationStorage> storageInstances = new HashMap<>();
-
/**
+ * Constructor.
+ *
* @param notificator Closure to execute when update from the storage is received.
+ * @param rootKeys Configuration root keys.
+ * @param validators Validators.
+ * @param storage Configuration storage.
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
*/
- public ConfigurationChanger(Notificator notificator) {
- this.notificator = notificator;
- }
-
- /**
- * Adds a single validator instance.
- * @param annotationType Annotation type for validated fields.
- * @param validator Validator instance for this annotation.
- * @param <A> Annotation type.
- */
- public <A extends Annotation> void addValidator(Class<A> annotationType, Validator<A, ?> validator) {
- validators
- .computeIfAbsent(annotationType, a -> new HashSet<>())
- .add(validator);
- }
-
- /**
- * Adds multiple validators instances.
- * @param annotationType Annotation type for validated fields.
- * @param validators Set of validator instancec for this annotation.
- */
- public void addValidators(Class<? extends Annotation> annotationType, Set<Validator<? extends Annotation, ?>> validators) {
- this.validators
- .computeIfAbsent(annotationType, a -> new HashSet<>())
- .addAll(validators);
- }
+ public ConfigurationChanger(
+ Notificator notificator,
+ Collection<RootKey<?, ?>> rootKeys,
+ Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators,
+ ConfigurationStorage storage
+ ) {
+ checkConfigurationType(rootKeys, storage);
- /**
- * Registers an additional root key.
- * @param rootKey Root key instance.
- */
- public void addRootKey(RootKey<?, ?> rootKey) {
- assert !storageInstances.containsKey(rootKey.type());
+ this.notificator = notificator;
+ this.validators = validators;
+ this.storage = storage;
- rootKeys.put(rootKey.key(), rootKey);
+ this.rootKeys = rootKeys.stream().collect(toMap(RootKey::key, identity()));
}
/**
* Created new {@code Node} object that corresponds to passed root keys root configuration node.
Review comment:
Please fix my typo, it should start with "Creates"
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -251,30 +225,27 @@ public void initialize(ConfigurationType storageType) throws ConfigurationValida
throw (ConfigurationChangeException)cause;
throw new ConfigurationChangeException(
- "Failed to write defalut configuration values into the storage " + configurationStorage.getClass(), e
+ "Failed to write defalut configuration values into the storage " + storage.getClass(), e
);
}
catch (InterruptedException e) {
throw new ConfigurationChangeException(
- "Failed to initialize configuration storage " + configurationStorage.getClass(), e
+ "Failed to initialize configuration storage " + storage.getClass(), e
);
}
}
/**
* Changes the configuration.
- * @param source Configuration source to create patch from.
- * @param storage Expected storage for the changes. If null, a derived storage will be used
- * unconditionaly.
+ *
+ * @param src Configuration source to create patch from.
* @return Future that is completed on change completion.
*/
- public CompletableFuture<Void> change(
- ConfigurationSource source,
- @Nullable ConfigurationStorage storage
- ) {
+ public CompletableFuture<Void> change(ConfigurationSource src) {
Set<ConfigurationType> storagesTypes = new HashSet<>();
ConstructableTreeNode collector = new ConstructableTreeNode() {
Review comment:
I think that this all monstrosity with the collector should be removed or heavily cut. We don't need this logic anymore since changer only knows about one storage
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -251,30 +225,27 @@ public void initialize(ConfigurationType storageType) throws ConfigurationValida
throw (ConfigurationChangeException)cause;
throw new ConfigurationChangeException(
- "Failed to write defalut configuration values into the storage " + configurationStorage.getClass(), e
+ "Failed to write defalut configuration values into the storage " + storage.getClass(), e
Review comment:
```suggestion
"Failed to write default configuration values into the storage " + storage.getClass(), e
```
##########
File path: modules/affinity/src/main/java/org/apache/ignite/internal/affinity/AffinityManager.java
##########
@@ -128,7 +128,7 @@ public AffinityManager(
* @return A future which will complete when the assignment is calculated.
*/
public CompletableFuture<Boolean> calculateAssignments(UUID tblId, String tblName) {
- TableConfiguration tblConfig = configurationMgr.configurationRegistry()
+ TableConfiguration tblConfig = configurationMgr.registry()
Review comment:
Can you please rename it back?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -302,19 +274,18 @@ public void initialize(ConfigurationType storageType) throws ConfigurationValida
)
);
}
-
- ConfigurationStorage actualStorage = storageInstances.get(storagesTypes.iterator().next());
-
- if (storage != null && storage != actualStorage) {
+ else if (storagesTypes.iterator().next() != storage.type()) {
return CompletableFuture.failedFuture(
new ConfigurationChangeException("Mismatched storage passed.")
);
}
- return changeInternally(source, actualStorage);
+ return changeInternally(src);
}
- /** Stop component. */
+ /**
+ * Stop component.
Review comment:
I saw that you changes multi-line comments to single-line somewhere (in RootKey, I think). What was your motivation?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -167,7 +155,7 @@ public void startStorageConfigurations(ConfigurationType storageType) {
* @throws IllegalArgumentException If {@code path} is not found in current configuration.
*/
public <T> T represent(List<String> path, ConfigurationVisitor<T> visitor) throws IllegalArgumentException {
- SuperRoot mergedSuperRoot = changer.mergedSuperRoot();
+ SuperRoot mergedSuperRoot = changer.superRoot();
Review comment:
Cool! We don't need merged superRoot anymore! Can you rename variable as well?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -251,30 +225,27 @@ public void initialize(ConfigurationType storageType) throws ConfigurationValida
throw (ConfigurationChangeException)cause;
throw new ConfigurationChangeException(
- "Failed to write defalut configuration values into the storage " + configurationStorage.getClass(), e
+ "Failed to write defalut configuration values into the storage " + storage.getClass(), e
);
}
catch (InterruptedException e) {
throw new ConfigurationChangeException(
- "Failed to initialize configuration storage " + configurationStorage.getClass(), e
+ "Failed to initialize configuration storage " + storage.getClass(), e
);
}
}
/**
* Changes the configuration.
- * @param source Configuration source to create patch from.
- * @param storage Expected storage for the changes. If null, a derived storage will be used
- * unconditionaly.
+ *
+ * @param src Configuration source to create patch from.
* @return Future that is completed on change completion.
*/
- public CompletableFuture<Void> change(
- ConfigurationSource source,
- @Nullable ConfigurationStorage storage
- ) {
+ public CompletableFuture<Void> change(ConfigurationSource src) {
Review comment:
Why do we need these unnecessary renames?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -60,38 +59,44 @@
* Class that handles configuration changes, by validating them, passing to storage and listening to storage updates.
*/
public abstract class ConfigurationChanger {
- /** */
+ /** Thread pool. */
private final ForkJoinPool pool = new ForkJoinPool(2);
- /** */
- private final Map<String, RootKey<?, ?>> rootKeys = new TreeMap<>();
+ /** Lazy annotations cache for configuration schema fields. */
+ private final Map<MemberKey, Annotation[]> cachedAnnotations = new ConcurrentHashMap<>();
+
+ /** Closure to execute when an update from the storage is received. */
+ private final Notificator notificator;
+
+ /** Root keys. Mapping: {@link RootKey#key()} -> identity (itself). */
+ private final Map<String, RootKey<?, ?>> rootKeys;
+
+ /** Validators. */
+ private final Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators;
- /** Map that has all the trees in accordance to their storages. */
- private final Map<ConfigurationType, StorageRoots> storagesRootsMap = new ConcurrentHashMap<>();
+ /** Configuration storage. */
+ private final ConfigurationStorage storage;
- /** Annotation classes mapped to validator objects. */
- private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** Storage trees. */
+ private volatile StorageRoots storageRoots;
/**
- * Closure interface to be used by the configuration changer. An instance of this closure is passed into the constructor and
- * invoked every time when there's an update from any of the storages.
+ * Closure interface to be used by the configuration changer. An instance of this closure is passed into
+ * the constructor and invoked every time when there's an update from any of the storages.
*/
@FunctionalInterface
public interface Notificator {
/**
* Invoked every time when the configuration is updated.
+ *
* @param oldRoot Old roots values. All these roots always belong to a single storage.
* @param newRoot New values for the same roots as in {@code oldRoot}.
* @param storageRevision Revision of the storage.
- * @return Not-null future that must signify when processing is completed. Exceptional completion is not
- * expected.
+ * @return Future that must signify when processing is completed. Exceptional completion is not expected.
*/
- @NotNull CompletableFuture<Void> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
+ CompletableFuture<Void> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
Review comment:
Why have you removed @NotNull?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -65,61 +63,54 @@
/** Root keys. */
private final Collection<RootKey<?, ?>> rootKeys;
- /** Validators. */
- private final Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators;
+ /** Configuration change handler. */
+ private final ConfigurationChanger changer;
- /** Configuration storages. */
- private final Collection<ConfigurationStorage> configurationStorages;
-
- /** */
- private volatile ConfigurationChanger changer;
-
- /** */
+ /** Configuration generator. */
private final ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
/**
* Constructor.
*
* @param rootKeys Configuration root keys.
* @param validators Validators.
- * @param configurationStorages Configuration Storages.
+ * @param storage Configuration storage.
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
*/
public ConfigurationRegistry(
Collection<RootKey<?, ?>> rootKeys,
Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators,
- Collection<ConfigurationStorage> configurationStorages
+ ConfigurationStorage storage
) {
+ checkConfigurationType(rootKeys, storage);
+
this.rootKeys = rootKeys;
- this.validators = validators;
- this.configurationStorages = configurationStorages;
- }
- /** {@inheritDoc} */
- @Override public void start() {
- this.changer = new ConfigurationChanger(this::notificator) {
+ Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators0 = new HashMap<>(validators);
+
+ validators0.put(Min.class, Set.of(new MinValidator()));
Review comment:
Technically this is a bug, set of validators for Min may already existed and you just erased it
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationManager.java
##########
@@ -19,97 +19,85 @@
import java.lang.annotation.Annotation;
import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import com.typesafe.config.ConfigFactory;
import com.typesafe.config.ConfigObject;
import org.apache.ignite.configuration.RootKey;
-import org.apache.ignite.configuration.annotation.ConfigurationType;
import org.apache.ignite.configuration.validation.Validator;
import org.apache.ignite.internal.configuration.hocon.HoconConverter;
import org.apache.ignite.internal.configuration.storage.ConfigurationStorage;
import org.apache.ignite.internal.manager.IgniteComponent;
+import static org.apache.ignite.internal.configuration.util.ConfigurationUtil.checkConfigurationType;
+
/**
* Configuration manager is responsible for handling configuration lifecycle and provides configuration API.
*/
public class ConfigurationManager implements IgniteComponent {
/** Configuration registry. */
- private final ConfigurationRegistry confRegistry;
-
- /** Type mapped to configuration storage. */
- private final Map<ConfigurationType, ConfigurationStorage> configurationStorages;
+ private final ConfigurationRegistry registry;
/**
- * The constructor.
+ * Constructor.
*
* @param rootKeys Configuration root keys.
* @param validators Validators.
- * @param configurationStorages Configuration storages.
+ * @param storage Configuration storage.
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
*/
public ConfigurationManager(
Collection<RootKey<?, ?>> rootKeys,
Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators,
- Collection<ConfigurationStorage> configurationStorages
+ ConfigurationStorage storage
) {
- HashMap<ConfigurationType, ConfigurationStorage> storageByType = new HashMap<>();
+ checkConfigurationType(rootKeys, storage);
- for (ConfigurationStorage storage : configurationStorages) {
- assert !storageByType.containsKey(storage.type()) : "Two or more storage have the same configuration type [type=" + storage.type() + ']';
-
- storageByType.put(storage.type(), storage);
- }
-
- this.configurationStorages = Map.copyOf(storageByType);
+ registry = new ConfigurationRegistry(rootKeys, validators, storage);
+ }
- confRegistry = new ConfigurationRegistry(rootKeys, validators, configurationStorages);
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Configuration root keys.
+ * @param storage Configuration storage.
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
+ */
+ public ConfigurationManager(Collection<RootKey<?, ?>> rootKeys, ConfigurationStorage storage) {
Review comment:
Is it used somewhere? I saw that tests use empty map for validators, right?
##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/configuration/SchemaConfigurationConverterTest.java
##########
@@ -68,9 +71,10 @@
@BeforeEach
public void createRegistry() throws ExecutionException, InterruptedException {
confRegistry = new ConfigurationRegistry(
- Collections.singleton(TablesConfiguration.KEY),
- Collections.singletonMap(TableValidator.class, Collections.singleton(SchemaTableValidatorImpl.INSTANCE)),
- Collections.singleton(new TestConfigurationStorage(ConfigurationType.DISTRIBUTED)));
+ List.of(TablesConfiguration.KEY),
+ Map.of(TableValidator.class, Set.of(SchemaTableValidatorImpl.INSTANCE)),
Review comment:
Please add the same thing in IgnitionImpl :)
--
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
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #273: IGNITE-15285 Splitting into ConfigurationManager managers (node and cluster)
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #273:
URL: https://github.com/apache/ignite-3/pull/273#discussion_r688565842
##########
File path: modules/rest/src/main/java/org/apache/ignite/rest/RestModule.java
##########
@@ -53,90 +55,88 @@
* Refer to default config file in resources for the example.
*/
public class RestModule implements IgniteComponent {
- /** */
+ /** Default port. */
public static final int DFLT_PORT = 10300;
- /** */
- private static final String CONF_URL = "/management/v1/configuration/";
+ /** Node configuration route. */
+ private static final String NODE_CFG_URL = "/management/v1/configuration/node/";
- /** */
+ /** Cluster configuration route. */
+ private static final String CLUSTER_CFG_URL = "/management/v1/configuration/cluster/";
+
+ /** Path parameter. */
private static final String PATH_PARAM = "selector";
/** Ignite logger. */
private final IgniteLogger LOG = IgniteLogger.forClass(RestModule.class);
- /** */
- private final ConfigurationRegistry sysConf;
+ /** Node configuration register. */
+ private final ConfigurationRegistry nodeCfgRegistry;
+
+ /** Presentation of node configuration. */
+ private final ConfigurationPresentation<String> nodeCfgPresentation;
- /** */
- private volatile ConfigurationPresentation<String> presentation;
+ /** Presentation of cluster configuration. */
+ private final ConfigurationPresentation<String> clusterCfgPresentation;
/**
* Creates a new instance of REST module.
*
* @param nodeCfgMgr Node configuration manager.
+ * @param clusterCfgMgr Cluster configuration manager.
*/
- public RestModule(ConfigurationManager nodeCfgMgr) {
- sysConf = nodeCfgMgr.configurationRegistry();
+ public RestModule(
+ ConfigurationManager nodeCfgMgr,
+ ConfigurationManager clusterCfgMgr
+ ) {
+ nodeCfgRegistry = nodeCfgMgr.registry();
+
+ nodeCfgPresentation = new HoconPresentation(nodeCfgMgr.registry());
+ clusterCfgPresentation = new HoconPresentation(clusterCfgMgr.registry());
}
/** {@inheritDoc} */
@Override public void start() {
- presentation = new HoconPresentation(sysConf);
-
var router = new Router();
router
- .get(CONF_URL, (req, resp) -> {
- resp.json(presentation.represent());
- })
- .get(CONF_URL + ":" + PATH_PARAM, (req, resp) -> {
- String cfgPath = req.queryParams().get(PATH_PARAM);
- try {
- resp.json(presentation.representByPath(cfgPath));
- }
- catch (IllegalArgumentException pathE) {
- ErrorResult eRes = new ErrorResult("CONFIG_PATH_UNRECOGNIZED", pathE.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- })
- .put(CONF_URL, HttpHeaderValues.APPLICATION_JSON, (req, resp) -> {
- try {
- presentation.update(
- req
- .request()
- .content()
- .readCharSequence(req.request().content().readableBytes(), StandardCharsets.UTF_8)
- .toString());
- }
- catch (IllegalArgumentException e) {
- ErrorResult eRes = new ErrorResult("INVALID_CONFIG_FORMAT", e.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- catch (ConfigurationValidationException e) {
- ErrorResult eRes = new ErrorResult("VALIDATION_EXCEPTION", e.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- catch (IgniteException e) {
- ErrorResult eRes = new ErrorResult("APPLICATION_EXCEPTION", e.getMessage());
-
- resp.status(BAD_REQUEST);
- resp.json(Map.of("error", eRes));
- }
- });
+ .get(
+ NODE_CFG_URL,
+ (req, resp) -> resp.json(nodeCfgPresentation.represent())
+ )
+ .get(
+ CLUSTER_CFG_URL,
+ (req, resp) -> resp.json(clusterCfgPresentation.represent())
+ )
+ .get(
+ NODE_CFG_URL + ":" + PATH_PARAM,
+ (req, resp) -> handleRepresentByPath(req, resp, nodeCfgPresentation)
+ )
+ .get(
+ CLUSTER_CFG_URL + ":" + PATH_PARAM,
+ (req, resp) -> handleRepresentByPath(req, resp, clusterCfgPresentation)
+ )
+ .put(
+ NODE_CFG_URL,
+ APPLICATION_JSON,
+ (req, resp) -> handleUpdate(req, resp, nodeCfgPresentation)
+ )
+ .put(
+ CLUSTER_CFG_URL,
+ APPLICATION_JSON,
+ (req, resp) -> handleUpdate(req, resp, clusterCfgPresentation)
+ );
startRestEndpoint(router);
}
- /** */
- private ChannelFuture startRestEndpoint(Router router) {
- RestView restConfigurationView = sysConf.getConfiguration(RestConfiguration.KEY).value();
+ /**
+ * Start endpoint.
+ *
+ * @param router Dispatcher of http requests.
+ */
+ private void startRestEndpoint(Router router) {
Review comment:
It is not used, why is it needed?
--
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