ivanzlenko commented on code in PR #6549: URL: https://github.com/apache/ignite-3/pull/6549#discussion_r2332167997
########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/RuntimeConfigurationException.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.cluster.management; + +import static org.apache.ignite.lang.ErrorGroups.CommonConfiguration.CONFIGURATION_VALIDATION_ERR; + +import org.apache.ignite.internal.lang.IgniteInternalException; + +/** + * Exception representing user input error. This is used to differentiate from system errors. + */ +public class RuntimeConfigurationException extends IgniteInternalException { Review Comment: Why not just InvalidConfigurationException or InvalidNodeConfigurationException? ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/responses/ValidationErrorResponse.java: ########## @@ -24,14 +24,17 @@ */ public class ValidationErrorResponse implements Serializable { private final String reason; + private final boolean configError; Review Comment: ```suggestion private final boolean invalidNodeConfig; ``` ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/RuntimeConfigurationException.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.cluster.management; + +import static org.apache.ignite.lang.ErrorGroups.CommonConfiguration.CONFIGURATION_VALIDATION_ERR; + +import org.apache.ignite.internal.lang.IgniteInternalException; + +/** + * Exception representing user input error. This is used to differentiate from system errors. Review Comment: JavaDoc doesn't correlate to the exception itself. ########## modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java: ########## @@ -1693,20 +1695,26 @@ private RuntimeException handleStartException(Throwable e) { var igniteException = new IgniteException(extractCodeFrom(e), errMsg, e); - // We log the exception as soon as possible to minimize the probability that it gets lost due to something like an OOM later. - LOG.error(errMsg, igniteException); + Throwable rootEx = unwrapRootCause(e); + if (rootEx instanceof RuntimeConfigurationException) { + LOG.warn("{}. Reason: {]", errMsg, rootEx.getMessage()); Review Comment: ```suggestion LOG.warn("{}. Reason: {}", errMsg, rootEx.getMessage()); ``` ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationResult.java: ########## @@ -25,23 +25,32 @@ public class ValidationResult { @Nullable private final String errorDescription; + private final boolean configError; - private ValidationResult(@Nullable String errorDescription) { + private ValidationResult(@Nullable String errorDescription, boolean configError) { this.errorDescription = errorDescription; + this.configError = configError; } /** * Creates a successful validation result. */ - public static ValidationResult successfulResult() { - return new ValidationResult(null); + static ValidationResult successfulResult() { + return new ValidationResult(null, false); + } + + /** + * Creates a failed validation result with a flag denoting whether caused by user error. Review Comment: JavaDoc wasn't changed ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/responses/ValidationErrorResponse.java: ########## @@ -24,14 +24,17 @@ */ public class ValidationErrorResponse implements Serializable { private final String reason; + private final boolean configError; /** * Creates a new response. * * @param reason Textual representation of the reason of join rejection. + * @param configError Flag denoting whether erroneous result is caused by config error. Review Comment: ```suggestion * @param configError Flag denoting whether erroneous result is caused by invalid node configuration. ``` ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationResult.java: ########## @@ -25,23 +25,32 @@ public class ValidationResult { @Nullable private final String errorDescription; + private final boolean configError; Review Comment: The same about field name applies ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java: ########## @@ -148,10 +149,18 @@ public CompletableFuture<Void> startJoinCluster(ClusterTag clusterTag, NodeAttri return raftService.run(command, RaftCommandRunner.NO_TIMEOUT) .thenAccept(response -> { if (response instanceof ValidationErrorResponse) { - throw new JoinDeniedException(((ValidationErrorResponse) response).reason()); + var validationErrorResponse = (ValidationErrorResponse) response; + + if (validationErrorResponse.isConfigError()) { Review Comment: Please add tests into ItClusterManagerTest covering this part ########## modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/responses/ValidationErrorResponse.java: ########## @@ -42,4 +45,13 @@ public ValidationErrorResponse(String reason) { public String reason() { return reason; } + + /** + * Flag marking this error as being caused by bad user input. Review Comment: JavaDoc doesn't match with the what method do -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
