You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "adityamukho (via GitHub)" <gi...@apache.org> on 2025/08/25 11:10:13 UTC
[PR] Reduced message verbosity on cluster init errors. [ignite-3]
adityamukho opened a new pull request, #6478:
URL: https://github.com/apache/ignite-3/pull/6478
https://issues.apache.org/jira/browse/IGNITE-26289
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "adityamukho (via GitHub)" <gi...@apache.org>.
adityamukho commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2301248090
##########
modules/cluster-management/src/test/java/org/apache/ignite/internal/cluster/management/ClusterInitializerTest.java:
##########
@@ -246,8 +246,7 @@ void testInitCancel() {
"cluster"
);
- String errorMessageFragment = String.format("Got error response from node \"%s\": foobar", cmgNode.name());
- assertThat(initFuture, willThrow(InternalInitException.class, errorMessageFragment));
+ assertThat(initFuture, willThrow(InternalInitException.class));
Review Comment:
The message prefix, which was being checked, has been removed.
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "adityamukho (via GitHub)" <gi...@apache.org>.
adityamukho commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2302825838
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
> Well, you call LOG.warn and it will print the message in the server logs =)
ok
> I don't understand, there are two similar messages in two branches of the same if statement....
I could do something like this:
```java
else if (!(response instanceof InitCompleteMessage || response instanceof PrepareInitCompleteMessage)) {
LOG.warn("Unexpected response from node \"{}\": {}", node.name(), response.getClass());
throw new InternalInitException(String.format("Unexpected error: %s", response.getClass()), true);
}
```
However, this too exposes an internal class name to the CLI. Is this ok? If not, what should the message be?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "adityamukho (via GitHub)" <gi...@apache.org>.
adityamukho commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2301244306
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
> 1. Why do we need this ofNullable stuff?
> 2. I think having the target node name in the logs would be useful, I think we should add a logging message at least.
> 3. Some lines below there's a Unexpected response from node message, should we fix it as well?
1. `InitErrorMessage.cause()` is marked as nullable, but the first parameter of the `InternalInitException` constructor isn't. It got flagged during commit analysis.
2. The node name was removed as part of removing the "Got error response..." message. This change makes that happen at the CLI, but unfortunately also at the server. I don't know if there is a way to selectively print the node name in the server log, but not show it at the CLI.
3. Perhaps, but it would perhaps be better to take it up as a general cleanup exercise in a separate ticket?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "adityamukho (via GitHub)" <gi...@apache.org>.
adityamukho commented on PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#issuecomment-3222801042
> I can see that you removed the "Join request denied" message everywhere, but what about other parts of the ticket?
Removed message "Got error response from node...". Node crash (or graceful shutdown) has its own ticket.
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo merged PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2298128066
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -225,7 +225,7 @@ public CompletableFuture<Void> initCluster(
e = e.getCause();
}
- LOG.info("Initialization failed [reason={}]", e, e.getMessage());
+ LOG.warn("Initialization failed [reason={}]", e.getMessage());
Review Comment:
This line is about printing the stacktrace to the logs, not to the end user. The logic here should be the following: if this error is caused by user actions (i.e. invalid user input) then no stacktrace is needed, otherwise, if this is caused by an internal server error, we definitely need a stacktrace
--
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
Re: [PR] Reduced message verbosity on cluster init errors. [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2297851794
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -225,7 +225,7 @@ public CompletableFuture<Void> initCluster(
e = e.getCause();
}
- LOG.info("Initialization failed [reason={}]", e, e.getMessage());
+ LOG.warn("Initialization failed [reason={}]", e.getMessage());
Review Comment:
What's the reasoning behind omitting stack trace here?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2301494239
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
> InitErrorMessage.cause() is marked as nullable, but the first parameter of the InternalInitException constructor isn't. It got flagged during commit analysis.
ok
> I don't know if there is a way to selectively print the node name in the server log, but not show it at the CLI.
Well, you call `LOG.warn` and it will print the message in the server logs =)
> Perhaps, but it would perhaps be better to take it up as a general cleanup exercise in a separate ticket?
I don't understand, there are two similar messages in two branches of the same `if` statement. Why did you change only one?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2300429826
##########
modules/cluster-management/src/test/java/org/apache/ignite/internal/cluster/management/ClusterInitializerTest.java:
##########
@@ -246,8 +246,7 @@ void testInitCancel() {
"cluster"
);
- String errorMessageFragment = String.format("Got error response from node \"%s\": foobar", cmgNode.name());
- assertThat(initFuture, willThrow(InternalInitException.class, errorMessageFragment));
+ assertThat(initFuture, willThrow(InternalInitException.class));
Review Comment:
why did you remove message checking altogether?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#issuecomment-3220455655
I can see that you removed the "Join request denied" message everywhere, but what about other parts of the ticket?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2300529941
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
Why not change it to something like: Initialization of node "node name" failed: errorResponse
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
We definitely should have a node name in the logs. That way it will be way easier to understand where the error occurred. It will be less so on production environment I guess, but in testing environment, and especially on TC it could be crucial.
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "adityamukho (via GitHub)" <gi...@apache.org>.
adityamukho commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2301244306
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
1. `InitErrorMessage.cause()` is marked as nullable, but the first parameter of the `InternalInitException` constructor isn't. It got flagged during commit analysis.
2. The node name was removed as part of removing the "Got error response..." message. This change makes that happen at the CLI, but unfortunately also at the server. I don't know if there is a way to selectively print the node name in the server log, but not show it at the CLI.
3. Perhaps, but it would perhaps be better to take it up as a general cleanup exercise in a separate ticket?
--
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
Re: [PR] Reduced message verbosity on cluster init errors. [ignite-3]
Posted by "adityamukho (via GitHub)" <gi...@apache.org>.
adityamukho commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2297957287
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -225,7 +225,7 @@ public CompletableFuture<Void> initCluster(
e = e.getCause();
}
- LOG.info("Initialization failed [reason={}]", e, e.getMessage());
+ LOG.warn("Initialization failed [reason={}]", e.getMessage());
Review Comment:
It flows from the broader question - should we show stacktraces to end users?
--
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
Re: [PR] Reduced message verbosity on cluster init errors. [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2297877531
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -225,7 +225,7 @@ public CompletableFuture<Void> initCluster(
e = e.getCause();
}
- LOG.info("Initialization failed [reason={}]", e, e.getMessage());
+ LOG.warn("Initialization failed [reason={}]", e.getMessage());
Review Comment:
I'm strongly against of removing stack trace here without good justification.
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#issuecomment-3223340979
@adityamukho could you please provide an example how init failures now look like when a user receives it as a response to a CLI command?
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2301490570
##########
modules/cluster-management/src/test/java/org/apache/ignite/internal/cluster/management/ClusterInitializerTest.java:
##########
@@ -246,8 +246,7 @@ void testInitCancel() {
"cluster"
);
- String errorMessageFragment = String.format("Got error response from node \"%s\": foobar", cmgNode.name());
- assertThat(initFuture, willThrow(InternalInitException.class, errorMessageFragment));
+ assertThat(initFuture, willThrow(InternalInitException.class));
Review Comment:
But there is also a message suffix that remained
--
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
Re: [PR] IGNITE-26289 Reduced message verbosity on cluster init errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #6478:
URL: https://github.com/apache/ignite-3/pull/6478#discussion_r2300428794
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -292,7 +293,7 @@ private CompletableFuture<Void> invokeMessage(Collection<ClusterNode> nodes, Net
var errorResponse = (InitErrorMessage) response;
throw new InternalInitException(
- String.format("Got error response from node \"%s\": %s", node.name(), errorResponse.cause()),
+ Optional.ofNullable(errorResponse.cause()).orElse(""),
Review Comment:
1. Why do we need this `ofNullable` stuff?
2. I think having the target node name in the logs would be useful, I think we should add a logging message at least.
3. Some lines below there's a `Unexpected response from node` message, should we fix it as well?
--
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