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 2020/12/24 08:30:55 UTC
[GitHub] [ignite-3] sergey-chugunov-1985 opened a new pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
sergey-chugunov-1985 opened a new pull request #14:
URL: https://github.com/apache/ignite-3/pull/14
This code still needs discussion and clarification but it raises important questions about abstractions for configuration presentation in different formats and update operations.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergey-chugunov-1985 commented on a change in pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
sergey-chugunov-1985 commented on a change in pull request #14:
URL: https://github.com/apache/ignite-3/pull/14#discussion_r562424326
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/json/JsonConverter.java
##########
@@ -22,6 +22,7 @@
import com.google.gson.Gson;
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergey-chugunov-1985 commented on a change in pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
sergey-chugunov-1985 commented on a change in pull request #14:
URL: https://github.com/apache/ignite-3/pull/14#discussion_r562418531
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/json/JsonConverter.java
##########
@@ -43,6 +44,15 @@
return gson.toJson(res);
}
+ /** {@inheritDoc} */
+ @Override public String rootName(String source) {
Review comment:
Yes, it cannot work with an update request packing more than one root. We may reconsider and change this in the future.
##########
File path: modules/runner/pom.xml
##########
@@ -64,6 +58,12 @@
<version>${project.version}</version>
<scope>compile</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.ignite</groupId>
+ <artifactId>ignite-rest</artifactId>
+ <version>3.0-SNAPSHOT</version>
Review comment:
Fixed.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/SystemConfiguration.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.presentation.json.JsonPresentation;
+
+/** */
+public class SystemConfiguration {
Review comment:
Agree, ConfiguratorRegistry name sounds more descriptive, renamed.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/SystemConfiguration.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.presentation.json.JsonPresentation;
+
+/** */
+public class SystemConfiguration {
+ /** */
+ private final Map<String, Configurator<?>> configs = new HashMap<>();
+
+ /** */
+ private final Map<String, ConfigurationTree<?, ?>> multirootConfiguration = new HashMap<>();
+
+ /** */
+ private final ConfigurationPresentation<String> presentation = new JsonPresentation(configs);
+
+ /** */
+ public <T extends DynamicConfiguration<?, ?, ?>> void registerConfigurator(Configurator<T> unitConfig) {
+ String key = unitConfig.getRoot().key();
+
+ configs.put(key, unitConfig);
+
+ multirootConfiguration.put(key, unitConfig.getRoot());
+ }
+
+ /** */
+ public <V, C, T extends ConfigurationTree<V, C>> T getConfiguration(RootKey<T> rootKey) {
+ return (T) multirootConfiguration.get(rootKey.key());
+ }
+
+ /** */
+ public ConfigurationPresentation<String> presentation() {
Review comment:
Removed this method, now rest module uses ConfigurationPresentation object directly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov closed pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
ibessonov closed pull request #14:
URL: https://github.com/apache/ignite-3/pull/14
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergey-chugunov-1985 commented on a change in pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
sergey-chugunov-1985 commented on a change in pull request #14:
URL: https://github.com/apache/ignite-3/pull/14#discussion_r562424190
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/SystemConfiguration.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.presentation.json.JsonPresentation;
+
+/** */
+public class SystemConfiguration {
+ /** */
+ private final Map<String, Configurator<?>> configs = new HashMap<>();
+
+ /** */
+ private final Map<String, ConfigurationTree<?, ?>> multirootConfiguration = new HashMap<>();
+
+ /** */
+ private final ConfigurationPresentation<String> presentation = new JsonPresentation(configs);
+
+ /** */
+ public <T extends DynamicConfiguration<?, ?, ?>> void registerConfigurator(Configurator<T> unitConfig) {
+ String key = unitConfig.getRoot().key();
+
+ configs.put(key, unitConfig);
+
+ multirootConfiguration.put(key, unitConfig.getRoot());
+ }
+
+ /** */
+ public <V, C, T extends ConfigurationTree<V, C>> T getConfiguration(RootKey<T> rootKey) {
Review comment:
We may stick with this approach until more complex modules are developed and see how it works against "configuration per root" approach. Then pick the best from API perspective.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] akalash commented on a change in pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
akalash commented on a change in pull request #14:
URL: https://github.com/apache/ignite-3/pull/14#discussion_r561038584
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/SystemConfiguration.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.presentation.json.JsonPresentation;
+
+/** */
+public class SystemConfiguration {
+ /** */
+ private final Map<String, Configurator<?>> configs = new HashMap<>();
+
+ /** */
+ private final Map<String, ConfigurationTree<?, ?>> multirootConfiguration = new HashMap<>();
+
+ /** */
+ private final ConfigurationPresentation<String> presentation = new JsonPresentation(configs);
+
+ /** */
+ public <T extends DynamicConfiguration<?, ?, ?>> void registerConfigurator(Configurator<T> unitConfig) {
+ String key = unitConfig.getRoot().key();
+
+ configs.put(key, unitConfig);
+
+ multirootConfiguration.put(key, unitConfig.getRoot());
+ }
+
+ /** */
+ public <V, C, T extends ConfigurationTree<V, C>> T getConfiguration(RootKey<T> rootKey) {
+ return (T) multirootConfiguration.get(rootKey.key());
+ }
+
+ /** */
+ public ConfigurationPresentation<String> presentation() {
Review comment:
Move to rest module?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/SystemConfiguration.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.presentation.json.JsonPresentation;
+
+/** */
+public class SystemConfiguration {
Review comment:
As I remember we thought about renaming it to something like ConfiguratorRegistry
##########
File path: modules/configuration/pom.xml
##########
@@ -40,6 +40,12 @@
<version>2.0.1.Final</version>
</dependency>
+ <dependency>
+ <groupId>com.google.code.gson</groupId>
+ <artifactId>gson</artifactId>
+ <version>2.8.6</version>
+ </dependency>
Review comment:
Move to rest module?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/Configurator.java
##########
@@ -118,6 +120,21 @@
return selector.select(root).value();
}
+ /**
+ *
+ */
+ public <CHANGE> Class<CHANGE> getChangeType() {
Review comment:
Maybe we should think about it. Instead of sharing the type, we can have a specific method 'set' with the raw key-value as input parameters. But I don't sure.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/SystemConfiguration.java
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.presentation.json.JsonPresentation;
+
+/** */
+public class SystemConfiguration {
+ /** */
+ private final Map<String, Configurator<?>> configs = new HashMap<>();
+
+ /** */
+ private final Map<String, ConfigurationTree<?, ?>> multirootConfiguration = new HashMap<>();
+
+ /** */
+ private final ConfigurationPresentation<String> presentation = new JsonPresentation(configs);
+
+ /** */
+ public <T extends DynamicConfiguration<?, ?, ?>> void registerConfigurator(Configurator<T> unitConfig) {
+ String key = unitConfig.getRoot().key();
+
+ configs.put(key, unitConfig);
+
+ multirootConfiguration.put(key, unitConfig.getRoot());
+ }
+
+ /** */
+ public <V, C, T extends ConfigurationTree<V, C>> T getConfiguration(RootKey<T> rootKey) {
Review comment:
If we are going to implement a configuration class per root then this method is not longer needed
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/RootKey.java
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+/** */
+public interface RootKey<T extends ConfigurationTree<?, ?>> {
Review comment:
If we are going to implement a configuration class per root then rootKey will not need anymore.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/FormatConverter.java
##########
@@ -27,6 +27,9 @@
/** */
Review comment:
Move whole class to rests module?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/json/JsonConverter.java
##########
@@ -22,6 +22,7 @@
import com.google.gson.Gson;
Review comment:
Move whole class to rest?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/json/JsonPresentation.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration.presentation.json;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationPresentation;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.Configurator;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.selector.BaseSelectors;
+
+/** */
+public class JsonPresentation implements ConfigurationPresentation<String> {
Review comment:
Move whole class to rest moduel?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sergey-chugunov-1985 commented on a change in pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
sergey-chugunov-1985 commented on a change in pull request #14:
URL: https://github.com/apache/ignite-3/pull/14#discussion_r562424252
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/FormatConverter.java
##########
@@ -27,6 +27,9 @@
/** */
Review comment:
Done.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/json/JsonPresentation.java
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration.presentation.json;
+
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationPresentation;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.Configurator;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.selector.BaseSelectors;
+
+/** */
+public class JsonPresentation implements ConfigurationPresentation<String> {
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #14: IGNITE-13836 First implementation of multi-root support for independent modules configurations
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #14:
URL: https://github.com/apache/ignite-3/pull/14#discussion_r560010689
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/ConfigurationDescription.java
##########
@@ -17,6 +17,7 @@
package org.apache.ignite.configuration.processor.internal;
+import com.squareup.javapoet.ClassName;
Review comment:
Import is out of order
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationPresentation.java
##########
@@ -0,0 +1,46 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.configuration;
+
+/** */
+public interface ConfigurationPresentation<R> {
+ /**
+ * Converts to presentation whole system configuration (all system modules are included).
+ *
+ * @return System configuration presentation converted to a given presentation type.
+ */
+ R represent();
+
+ /**
+ * Converts to presentation only a fraction of system configuration defined by given path.
+ *
+ * If null path is passed method should fall back to returning whole system configuration.
+ *
+ * @param path Path to requested configuration in configuration tree or {@code null}.
+ *
+ * @return Requested configuration fraction or whole configuration if {@code null} was passed.
+ */
+ R representByPath(String path);
+
+ /**
+ * Converts and applies configuration update request to system configuration.
+ *
+ * @param configUpdate Configuration update request in representation form.
+ */
+ void update(R configUpdate);
Review comment:
Looks questionable, maybe we'll revisit this method in the future.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/presentation/json/JsonConverter.java
##########
@@ -43,6 +44,15 @@
return gson.toJson(res);
}
+ /** {@inheritDoc} */
+ @Override public String rootName(String source) {
Review comment:
This method assumes that there's only one root, right?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/Configurator.java
##########
@@ -118,6 +120,21 @@
return selector.select(root).value();
}
+ /**
+ *
+ */
+ public <CHANGE> Class<CHANGE> getChangeType() {
Review comment:
You decided not to move "CHANGE" class into the class signature, but why?
##########
File path: modules/runner/pom.xml
##########
@@ -64,6 +58,12 @@
<version>${project.version}</version>
<scope>compile</scope>
</dependency>
+ <dependency>
+ <groupId>org.apache.ignite</groupId>
+ <artifactId>ignite-rest</artifactId>
+ <version>3.0-SNAPSHOT</version>
Review comment:
${project.version}
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org