ivanzlenko commented on code in PR #6549:
URL: https://github.com/apache/ignite-3/pull/6549#discussion_r2321111199
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/responses/ValidationErrorResponse.java:
##########
@@ -42,4 +39,8 @@ public ValidationErrorResponse(String reason) {
public String reason() {
return reason;
}
+
+ public boolean isUserError() {
Review Comment:
Please add javaDoc for this method
##########
modules/api/src/main/java/org/apache/ignite/lang/InvalidUserInputException.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.lang;
+
+import static org.apache.ignite.lang.ErrorGroups.Common.ILLEGAL_ARGUMENT_ERR;
+
+import java.util.UUID;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Exception representing user input error. This is used to differentiate from
system errors.
+ */
+public class InvalidUserInputException extends IgniteException {
+ public InvalidUserInputException(String message) {
+ super(ILLEGAL_ARGUMENT_ERR, message);
+ }
+
+ @SuppressWarnings("unused")
Review Comment:
Let's remove this suppression.
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1690,23 +1692,28 @@ private void checkSelfInTopology(
private RuntimeException handleStartException(Throwable e) {
String errMsg = "Unable to start [node=" + name + "]";
-
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);
+ var rootEx = unwrapRootCause(e);
Review Comment:
Please use correct type declaration instead of var where it is not obvious
what actual type could be.
##########
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 userError;
Review Comment:
User error is too broad and will be hard to comprehend for developers and
customers why we have it.
It's better to name it configurationError or something like this.
Or maybe even reverse logic of this field and indicate "system" errors,
instead of "user"
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationResult.java:
##########
@@ -59,4 +68,11 @@ String errorDescription() {
return errorDescription;
}
+
+ /**
+ * Returns flag denoting whether errorneous result is caused by user error.
Review Comment:
erroneous
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java:
##########
@@ -148,7 +149,15 @@ 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.isUserError()) {
+ var invalidUserInputException = new
InvalidUserInputException(validationErrorResponse.reason());
+
+ throw new
JoinDeniedException(invalidUserInputException.code(),
invalidUserInputException);
+
Review Comment:
```suggestion
```
##########
modules/api/src/main/java/org/apache/ignite/lang/InvalidUserInputException.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.lang;
+
+import static org.apache.ignite.lang.ErrorGroups.Common.ILLEGAL_ARGUMENT_ERR;
+
+import java.util.UUID;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Exception representing user input error. This is used to differentiate from
system errors.
+ */
+public class InvalidUserInputException extends IgniteException {
Review Comment:
Let's move this exception into cluster-management module
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/responses/ValidationErrorResponse.java:
##########
@@ -24,14 +24,11 @@
*/
public class ValidationErrorResponse implements Serializable {
private final String reason;
+ private final boolean userError;
- /**
Review Comment:
Why javaDoc got removed?
##########
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 userError;
- private ValidationResult(@Nullable String errorDescription) {
+ private ValidationResult(@Nullable String errorDescription, boolean
userError) {
this.errorDescription = errorDescription;
+ this.userError = userError;
}
/**
* Creates a successful validation result.
*/
public static ValidationResult successfulResult() {
- return new ValidationResult(null);
+ return new ValidationResult(null, false);
+ }
+
+ /**
+ * Creates a failed validation result with a flag denoting whether caused
by user error.
+ */
+ public static ValidationResult errorResult(String errorDescription,
boolean userError) {
+ return new ValidationResult(errorDescription, userError);
}
/**
* Creates a failed validation result.
*/
public static ValidationResult errorResult(String errorDescription) {
- return new ValidationResult(errorDescription);
+ return new ValidationResult(errorDescription, false);
Review Comment:
Let's make this method package-private
##########
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 userError;
- private ValidationResult(@Nullable String errorDescription) {
+ private ValidationResult(@Nullable String errorDescription, boolean
userError) {
this.errorDescription = errorDescription;
+ this.userError = userError;
}
/**
* Creates a successful validation result.
*/
public static ValidationResult successfulResult() {
- return new ValidationResult(null);
+ return new ValidationResult(null, false);
+ }
+
+ /**
+ * Creates a failed validation result with a flag denoting whether caused
by user error.
+ */
+ public static ValidationResult errorResult(String errorDescription,
boolean userError) {
Review Comment:
Let's replace this method with signature without boolean. The only usage we
have is with 'true' value, so it definitely could be replaced.
##########
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 userError;
- private ValidationResult(@Nullable String errorDescription) {
+ private ValidationResult(@Nullable String errorDescription, boolean
userError) {
this.errorDescription = errorDescription;
+ this.userError = userError;
}
/**
* Creates a successful validation result.
*/
public static ValidationResult successfulResult() {
Review Comment:
let's make it package-private
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/responses/ValidationErrorResponse.java:
##########
@@ -24,14 +24,11 @@
*/
public class ValidationErrorResponse implements Serializable {
private final String reason;
+ private final boolean userError;
Review Comment:
The same about user vs system goes for this field.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/ValidationResult.java:
##########
@@ -59,4 +68,11 @@ String errorDescription() {
return errorDescription;
}
+
+ /**
+ * Returns flag denoting whether errorneous result is caused by user error.
+ */
+ public boolean isUserError() {
Review Comment:
Let's make it package private
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1690,23 +1692,28 @@ private void checkSelfInTopology(
private RuntimeException handleStartException(Throwable e) {
String errMsg = "Unable to start [node=" + name + "]";
-
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);
+ var rootEx = unwrapRootCause(e);
+ if (rootEx instanceof InvalidUserInputException) {
+ LOG.warn(String.format("%s. Reason: %s", errMsg,
rootEx.getMessage()));
Review Comment:
You do not need String.format here. Logger has its own formatter.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftGroupListener.java:
##########
@@ -193,7 +193,8 @@ public void onWrite(Iterator<CommandClosure<WriteCommand>>
iterator) {
} else if (command instanceof JoinRequestCommand) {
ValidationResult response =
validateNode((JoinRequestCommand) command);
- clo.result(response.isValid() ? null : new
ValidationErrorResponse(response.errorDescription()));
+ clo.result(
Review Comment:
```suggestion
clo.result(response.isValid()
? null
: new
ValidationErrorResponse(response.errorDescription(), response.isUserError())
);
```
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftGroupListener.java:
##########
@@ -202,7 +203,8 @@ public void onWrite(Iterator<CommandClosure<WriteCommand>>
iterator) {
onLogicalTopologyChanged.accept(clo.term());
}
- clo.result(response.isValid() ? null : new
ValidationErrorResponse(response.errorDescription()));
+ clo.result(
Review Comment:
```suggestion
clo.result(response.isValid()
? null
: new
ValidationErrorResponse(response.errorDescription(), response.isUserError())
);
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1690,23 +1692,28 @@ private void checkSelfInTopology(
private RuntimeException handleStartException(Throwable e) {
String errMsg = "Unable to start [node=" + name + "]";
-
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);
+ var rootEx = unwrapRootCause(e);
Review Comment:
```suggestion
var rootEx = unwrapRootCause(e);
```
##########
modules/api/src/main/java/org/apache/ignite/lang/InvalidUserInputException.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.lang;
+
+import static org.apache.ignite.lang.ErrorGroups.Common.ILLEGAL_ARGUMENT_ERR;
+
+import java.util.UUID;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Exception representing user input error. This is used to differentiate from
system errors.
+ */
+public class InvalidUserInputException extends IgniteException {
Review Comment:
Also I'm not sure whether InvalidUserInputException is a good name. It is
not an invalid user input per say. We have misconfiguration on our hands
rather.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java:
##########
@@ -148,7 +149,15 @@ 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;
Review Comment:
```suggestion
var validationErrorResponse =
(ValidationErrorResponse) response;
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1690,23 +1692,28 @@ private void checkSelfInTopology(
private RuntimeException handleStartException(Throwable e) {
String errMsg = "Unable to start [node=" + name + "]";
-
Review Comment:
Why this line got removed?
##########
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 userError;
- private ValidationResult(@Nullable String errorDescription) {
+ private ValidationResult(@Nullable String errorDescription, boolean
userError) {
this.errorDescription = errorDescription;
+ this.userError = userError;
}
/**
* Creates a successful validation result.
*/
public static ValidationResult successfulResult() {
- return new ValidationResult(null);
+ return new ValidationResult(null, false);
+ }
+
+ /**
+ * Creates a failed validation result with a flag denoting whether caused
by user error.
+ */
+ public static ValidationResult errorResult(String errorDescription,
boolean userError) {
Review Comment:
Also let's make it package private
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
