You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/07/29 13:32:58 UTC
[GitHub] [ignite-3] valepakh opened a new pull request, #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
valepakh opened a new pull request, #961:
URL: https://github.com/apache/ignite-3/pull/961
This will add --port, --rest-port and --join parameters to simplify starting nodes especially for local testing.
https://issues.apache.org/jira/browse/IGNITE-17347
--
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] sk0x50 commented on a diff in pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r943375896
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/builtins/node/NodeManager.java:
##########
@@ -136,6 +137,9 @@ public RunningNode start(
if (srvCfg != null) {
cmdArgs.add("--config");
cmdArgs.add(srvCfg.toAbsolutePath().toString());
+ } else if (srvCfgStr != null) {
+ cmdArgs.add("--configStr");
Review Comment:
Perhaps, `config-overrides` would cleaner. What do you think?
--
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] valepakh commented on pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
valepakh commented on PR #961:
URL: https://github.com/apache/ignite-3/pull/961#issuecomment-1211701123
I changed CLI runner parameters to `--config-path` and `--config-string` and implemented overriding config file with parameters.
Also I improved the IgniteCliInterfaceTest so that the mockserver only starts once, this significantly improved performance when running on my local machine in IDEA.
--
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] sk0x50 commented on a diff in pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r943380546
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/builtins/node/NodeManager.java:
##########
@@ -136,6 +137,9 @@ public RunningNode start(
if (srvCfg != null) {
cmdArgs.add("--config");
cmdArgs.add(srvCfg.toAbsolutePath().toString());
+ } else if (srvCfgStr != null) {
+ cmdArgs.add("--configStr");
Review Comment:
Looks like I was wrong. My last comment does not make sense. Please ignore.
--
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 #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r934361402
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/NodeCommandSpec.java:
##########
@@ -100,12 +123,36 @@ public Integer call() {
out.println(tbl);
return 0;
}
+
+ private Path getConfigPath() {
+ return configOptions != null ? configOptions.configPath : null;
+ }
+
+ private String getConfigStr() {
+ if (configOptions == null || configOptions.configPath != null) {
+ return null;
+ }
+ // port is required
+ StringBuilder sb = new StringBuilder();
Review Comment:
I think it is better to use `Config` class from `typesafe` lib to construct a configuration representation.
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/NodeCommandSpec.java:
##########
@@ -62,12 +65,31 @@ public static class StartNodeCommandSpec extends BaseCommand implements Callable
private NodeManager nodeMgr;
/** Consistent id, which will be used by new node. */
- @CommandLine.Parameters(paramLabel = "name", description = "Name of the new node")
+ @Parameters(paramLabel = "name", description = "Name of the new node")
public String nodeName;
- /** Path to node config. */
- @CommandLine.Option(names = "--config", description = "Configuration file to start the node with")
- private Path configPath;
+ @ArgGroup
+ private ConfigOptions configOptions;
+
+ private static class ConfigOptions {
+ @ArgGroup(exclusive = false)
Review Comment:
Could you add an unit test to verify that it is truly exclusive?
--
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] asfgit closed pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
URL: https://github.com/apache/ignite-3/pull/961
--
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] valepakh commented on a diff in pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r942681580
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/NodeCommandSpec.java:
##########
@@ -102,12 +130,32 @@ public Integer call() {
out.println(tbl);
return 0;
}
+
+ private Path getConfigPath() {
+ return configOptions != null ? configOptions.configPath : null;
+ }
+
+ private String getConfigStr() {
Review Comment:
Yes I think this is possible and will be helpful.
--
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] valepakh commented on a diff in pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r942680145
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/builtins/node/NodeManager.java:
##########
@@ -136,6 +137,9 @@ public RunningNode start(
if (srvCfg != null) {
cmdArgs.add("--config");
cmdArgs.add(srvCfg.toAbsolutePath().toString());
+ } else if (srvCfgStr != null) {
+ cmdArgs.add("--configStr");
Review Comment:
`--config-file` and `--config-string` maybe?
--
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] sk0x50 commented on a diff in pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r942653841
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/builtins/node/NodeManager.java:
##########
@@ -136,6 +137,9 @@ public RunningNode start(
if (srvCfg != null) {
cmdArgs.add("--config");
cmdArgs.add(srvCfg.toAbsolutePath().toString());
+ } else if (srvCfgStr != null) {
+ cmdArgs.add("--configStr");
Review Comment:
`--configStr` does not look descriptive to be honest :) By the way, do we use camel case for commands? Perhaps it should be named `config-str`, for instance see at `work-dir`.
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/NodeCommandSpec.java:
##########
@@ -102,12 +130,32 @@ public Integer call() {
out.println(tbl);
return 0;
}
+
+ private Path getConfigPath() {
+ return configOptions != null ? configOptions.configPath : null;
+ }
+
+ private String getConfigStr() {
Review Comment:
Well, looks like these parameters (port, rest-port etc) cannot be provided with a configuration file. Right?
It seems to me, that ability to start two nodes with the same configuration file and overridden value of `port` would be very convenient. I mean the following:
`node start nodeName --config \path_to_configuration_file --port 12345`
What do you think?
--
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