-
Notifications
You must be signed in to change notification settings - Fork 135
IGNITE-14413 Added start of new node from cli interface. #157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
Outdated
Show resolved
Hide resolved
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
Outdated
Show resolved
Hide resolved
modules/runner/src/main/java/org/apache/ignite/internal/app/IgnitionImpl.java
Show resolved
Hide resolved
modules/runner/src/main/java/org/apache/ignite/app/IgniteCliRunner.java
Outdated
Show resolved
Hide resolved
| return new Args(args[0], null); | ||
| else if (args.length == 3) { | ||
| if ("--config".equals(args[0])) | ||
| return new Args(args[2], Path.of(args[1])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be some validation? Path.of could throw at least IllegalArgumentException or FileSystemNotFoundException, should we catch such cases and rethrow ParseException?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed for InvalidPathException. Looks like other types of runtime exceptions are not relevant for catching here.
| } | ||
| catch (Args.ParseException e) { | ||
| System.out.println(e.getMessage()); | ||
| System.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line. Also, should we write here some clarification what happened before e.getMessage()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's so tricky to improve the messaging for all types of issues without more complex argument parsing logic. I think that usage message + [some details] (for --config path parsing) is ok for current arguments. WDYT?
Anyway, error reporting updated, you can check the current aprroach.
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/IgnitionTest.java
Outdated
Show resolved
Hide resolved
| public String name() throws IgniteInternalCheckedException { | ||
| synchronized (mux) { | ||
| try { | ||
| return new String(vaultService.get(NODE_NAME).get().value()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but should we consider some caching here? Seems that name() might be called frequently, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, not sure too :) appliedRevision can have the same issue, maybe we should fix it in the scope of another issue?
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
Outdated
Show resolved
Hide resolved
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
Outdated
Show resolved
Hide resolved
modules/runner/src/main/java/org/apache/ignite/internal/app/IgnitionImpl.java
Show resolved
Hide resolved
…aultManager.java Co-authored-by: Mirza Aliev <alievmirza@gmail.com>
Signed-off-by: Slava Koptilin <slava.koptilin@gmail.com> (cherry picked from commit 44d69d3)
No description provided.