You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Flaugh24 (via GitHub)" <gi...@apache.org> on 2023/05/10 19:36:55 UTC
[GitHub] [ignite-3] Flaugh24 opened a new pull request, #2058: IGNITE-19315 Validate node configuration on node start
Flaugh24 opened a new pull request, #2058:
URL: https://github.com/apache/ignite-3/pull/2058
(no comment)
--
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] Flaugh24 commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "Flaugh24 (via GitHub)" <gi...@apache.org>.
Flaugh24 commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1192445068
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.configuration;
+
+import org.apache.ignite.lang.ErrorGroups.NodeConfiguration;
+import org.apache.ignite.lang.IgniteException;
+
+/**
+ * Throws when node bootstrap configuration file failed to parse.
+ */
+public class NodeConfigParseException extends IgniteException {
Review Comment:
We already have NodeConfigCreateException, NodeConfigReadException, NodeConfigWriteException
--
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] PakhomovAlexander commented on pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#issuecomment-1545542771
Looks ok to me. I would appreciate it if @ibessonov takes a look.
--
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] Flaugh24 commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "Flaugh24 (via GitHub)" <gi...@apache.org>.
Flaugh24 commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1194920343
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java:
##########
@@ -137,10 +139,18 @@ void missingPolymorphicExtension() {
@Test
void testComplicatedPolymorphicConfig() throws Exception {
+
+ Map<String, Serializable> bootstrapConfig = Map.of(
Review Comment:
Configuration validation happens on org.apache.ignite.internal.configuration.ConfigurationChanger#start. Nulls are not allowed in the configuration, so we have to initialize null fields
--
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] PakhomovAlexander merged pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander merged PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058
--
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] Flaugh24 commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "Flaugh24 (via GitHub)" <gi...@apache.org>.
Flaugh24 commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1192081020
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * General integration tests for node configuration file.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeConfigurationFileTest {
+
+ @Test
+ public void wrongConfigurationFormat(TestInfo testInfo, @WorkDirectory Path workDir) {
+ String config = UUID.randomUUID().toString();
+
+ IgniteException igniteException = assertThrows(
+ IgniteException.class,
+ () -> TestIgnitionManager.start(testNodeName(testInfo, 0), config, workDir)
+ );
+ assertThat(igniteException.getCause(), is(instanceOf(NodeConfigParseException.class)));
+ assertThat(igniteException.getCause().getMessage(), containsString("Failed to parse config content from file "));
Review Comment:
See my comment above
--
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] Flaugh24 commented on pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "Flaugh24 (via GitHub)" <gi...@apache.org>.
Flaugh24 commented on PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#issuecomment-1542880808
https://issues.apache.org/jira/browse/IGNITE-19315
--
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 diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1192266124
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -274,11 +274,30 @@ public void start() throws ConfigurationChangeException {
//Workaround for distributed configuration.
addDefaults(superRoot);
+ // Validate the restored configuration.
+ validateConfiguration(defaultConfiguration(), superRoot);
+
storageRoots = new StorageRoots(superRoot, data.changeId());
storage.registerConfigurationListener(configurationStorageListener());
}
+ /**
+ * Creates an empty configuration with defaults.
+ *
+ * @return Default configuration.
+ */
+ private SuperRoot defaultConfiguration() {
Review Comment:
There may be no default values, what happens then?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -95,12 +96,12 @@ public static class FirstConfigurationSchema {
*/
@Config
public static class SecondConfigurationSchema {
- @Value
+ @Value(hasDefault = true)
@Immutable
- public int intCfg;
+ public int intCfg = 0;
- @Value
- public String strCfg;
+ @Value(hasDefault = true)
+ public String strCfg = "";
}
Review Comment:
I think we can add fields with default values instead of adding default values for the current ones.
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java:
##########
@@ -370,16 +370,16 @@ public static class FourthPolymorphicConfigurationSchema {
@PolymorphicId(hasDefault = true)
public String typeId = "fourth0";
- @Value
- public String strVal;
+ @Value(hasDefault = true)
+ public String strVal = "";
}
/**
* First {@link FourthPolymorphicConfigurationSchema} extension.
*/
@PolymorphicConfigInstance("fourth0")
public static class Fourth0PolymorphicConfigurationSchema extends FourthPolymorphicConfigurationSchema {
- @Value
- public int intVal;
+ @Value(hasDefault = true)
Review Comment:
Same
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.configuration;
+
+import org.apache.ignite.lang.ErrorGroups.NodeConfiguration;
+import org.apache.ignite.lang.IgniteException;
+
+/**
+ * Throws when node bootstrap configuration file failed to parse.
+ */
+public class NodeConfigParseException extends IgniteException {
Review Comment:
```suggestion
public class ConfigurationParseException extends IgniteException {
```
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java:
##########
@@ -370,16 +370,16 @@ public static class FourthPolymorphicConfigurationSchema {
@PolymorphicId(hasDefault = true)
public String typeId = "fourth0";
- @Value
- public String strVal;
+ @Value(hasDefault = true)
Review Comment:
Same
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.configuration;
+
+import org.apache.ignite.lang.ErrorGroups.NodeConfiguration;
+import org.apache.ignite.lang.IgniteException;
+
+/**
+ * Throws when node bootstrap configuration file failed to parse.
+ */
+public class NodeConfigParseException extends IgniteException {
+
+ public NodeConfigParseException(String msg, Throwable cause) {
Review Comment:
Missing javadoc
--
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] sashapolo commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1193129938
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -274,6 +274,9 @@ public void start() throws ConfigurationChangeException {
//Workaround for distributed configuration.
addDefaults(superRoot);
+ // Validate the restored configuration.
+ validateConfiguration(superRoot, superRoot);
Review Comment:
This looks strange to me, since old values and new values will be the same. Shouldn't we use null or an empty root as the old value?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java:
##########
@@ -370,16 +370,16 @@ public static class FourthPolymorphicConfigurationSchema {
@PolymorphicId(hasDefault = true)
public String typeId = "fourth0";
- @Value
- public String strVal;
+ @Value(hasDefault = true)
Review Comment:
Why are these changes needed?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Integration tests for the node configuration file.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeConfigurationFileTest {
+
+ @Test
+ public void wrongConfigurationFormat(TestInfo testInfo, @WorkDirectory Path workDir) {
+ String config = UUID.randomUUID().toString();
+
+ IgniteException igniteException = assertThrows(
+ IgniteException.class,
+ () -> TestIgnitionManager.start(testNodeName(testInfo, 0), config, workDir)
+ );
+ assertThat(igniteException.getCause(), is(instanceOf(NodeConfigParseException.class)));
+ assertThat(igniteException.getCause().getMessage(), containsString("Failed to parse config content from file "));
Review Comment:
Should we also check that config file location is present in the message?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Integration tests for the node configuration file.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeConfigurationFileTest {
+
+ @Test
+ public void wrongConfigurationFormat(TestInfo testInfo, @WorkDirectory Path workDir) {
Review Comment:
`workDir` can be converted into a class field
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Integration tests for the node configuration file.
Review Comment:
These tests are clearly not for testing "the node configuration file", can you please elaborate on this comment?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -242,7 +242,7 @@ private Function<String, RootInnerNode> rootCreator() {
* Start component.
*/
// ConfigurationChangeException, really?
Review Comment:
This comment is now obsolete
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -95,12 +96,12 @@ public static class FirstConfigurationSchema {
*/
@Config
public static class SecondConfigurationSchema {
- @Value
+ @Value(hasDefault = true)
@Immutable
- public int intCfg;
+ public int intCfg = 0;
- @Value
- public String strCfg;
+ @Value(hasDefault = true)
+ public String strCfg = "";
}
Review Comment:
Why are these changes needed in the first place?
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/NodeConfigParseException.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.configuration;
+
+import org.apache.ignite.lang.ErrorGroups.NodeConfiguration;
+import org.apache.ignite.lang.IgniteException;
+
+/**
+ * Throws when node bootstrap configuration file failed to parse.
Review Comment:
```suggestion
* Exception that gets thrown when a node bootstrap configuration file is malformed.
```
--
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] PakhomovAlexander commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1190873409
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -274,11 +274,30 @@ public void start() throws ConfigurationChangeException {
//Workaround for distributed configuration.
addDefaults(superRoot);
+ // Validate the restored configuration
Review Comment:
```suggestion
// Validate the restored configuration.
```
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * General integration tests for node configuration file.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeConfigurationFileTest {
+
+ @Test
+ public void wrongConfigurationFormat(TestInfo testInfo, @WorkDirectory Path workDir) {
+ String config = UUID.randomUUID().toString();
+
+ IgniteException igniteException = assertThrows(
+ IgniteException.class,
+ () -> TestIgnitionManager.start(testNodeName(testInfo, 0), config, workDir)
+ );
+ assertThat(igniteException.getCause(), is(instanceOf(NodeConfigParseException.class)));
+ assertThat(igniteException.getCause().getMessage(), containsString("Failed to parse config content from file "));
Review Comment:
from what file do we fail to parse the config? I don't see any file here.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * General integration tests for node configuration file.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeConfigurationFileTest {
Review Comment:
Why is there `File` in the class name? I can find tests without any files. Would you drop this from the class name?
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * General integration tests for node configuration file.
Review Comment:
```suggestion
* Integration tests for the node configuration file.
```
--
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] Flaugh24 commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "Flaugh24 (via GitHub)" <gi...@apache.org>.
Flaugh24 commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1192080580
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItNodeConfigurationFileTest.java:
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.internal.configuration;
+
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.assertThrowsWithCause;
+import static org.apache.ignite.internal.testframework.IgniteTestUtils.testNodeName;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import java.nio.file.Path;
+import java.util.UUID;
+import org.apache.ignite.configuration.validation.ConfigurationValidationException;
+import org.apache.ignite.internal.testframework.TestIgnitionManager;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.lang.IgniteException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * General integration tests for node configuration file.
+ */
+@ExtendWith(WorkDirectoryExtension.class)
+public class ItNodeConfigurationFileTest {
Review Comment:
org.apache.ignite.internal.testframework.TestIgnitionManager#start creates the configuration file in a work directory, which is going to be read by Ignite
--
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] sashapolo commented on a diff in pull request #2058: IGNITE-19315 Validate node configuration on node start
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2058:
URL: https://github.com/apache/ignite-3/pull/2058#discussion_r1194862473
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationRegistryTest.java:
##########
@@ -137,10 +139,18 @@ void missingPolymorphicExtension() {
@Test
void testComplicatedPolymorphicConfig() throws Exception {
+
+ Map<String, Serializable> bootstrapConfig = Map.of(
Review Comment:
What is this for?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -274,6 +274,9 @@ public void start() throws ConfigurationChangeException {
//Workaround for distributed configuration.
addDefaults(superRoot);
+ // Validate the restored configuration.
+ validateConfiguration(new SuperRoot(null), superRoot);
Review Comment:
`null` is not expected here, maybe `rootCreator()` is a better alternative
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -242,7 +242,7 @@ private Function<String, RootInnerNode> rootCreator() {
* Start component.
*/
// ConfigurationChangeException, really?
Review Comment:
You should remove this comment
--
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