alievmirza commented on a change in pull request #157:
URL: https://github.com/apache/ignite-3/pull/157#discussion_r644873624
##########
File path:
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
##########
@@ -171,6 +175,31 @@ public boolean bootstrapped() {
}
}
+ /**
+ * Persist node name to the vault.
+ *
+ * @param name Node name to persist.
+ * @return Future representing pending completion of the operation.
Couldn't be {@code null}.
+ */
+ @NotNull public CompletableFuture<Void> putName(@NotNull String name) {
+ return put(NODE_NAME, name.getBytes(StandardCharsets.UTF_8));
+ }
+
+ /**
+ * @return Node name, if was stored earlier.
+ * @throws IgniteInternalCheckedException If couldn't get node name from
the vault.
+ */
+ public String name() throws IgniteInternalCheckedException {
+ synchronized (mux) {
+ try {
+ return new String(vaultService.get(NODE_NAME).get().value());
Review comment:
Please specify charset StandardCharsets.UTF_8
##########
File path:
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
##########
@@ -171,6 +175,31 @@ public boolean bootstrapped() {
}
}
+ /**
+ * Persist node name to the vault.
+ *
+ * @param name Node name to persist.
+ * @return Future representing pending completion of the operation.
Couldn't be {@code null}.
+ */
+ @NotNull public CompletableFuture<Void> putName(@NotNull String name) {
+ return put(NODE_NAME, name.getBytes(StandardCharsets.UTF_8));
+ }
+
+ /**
+ * @return Node name, if was stored earlier.
+ * @throws IgniteInternalCheckedException If couldn't get node name from
the vault.
+ */
+ public String name() throws IgniteInternalCheckedException {
+ synchronized (mux) {
+ try {
+ return new String(vaultService.get(NODE_NAME).get().value());
Review comment:
I would wait for future's get outside of the synchronized block
##########
File path:
modules/runner/src/main/java/org/apache/ignite/app/IgniteCliRunner.java
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.app;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.ignite.internal.app.IgnitionImpl;
+
+/**
+ * The main entry point for run new Ignite node from CLI toolchain.
+ */
+public class IgniteCliRunner {
+ /**
+ * Main method for run new Ignite node.
+ *
+ * @param args CLI args to start new node.
+ * @throws IOException if any issues with reading config file.
+ */
+ public static void main(String[] args) throws IOException {
+ Args parsedArgs = null;
+
+ try {
+ parsedArgs = Args.parseArgs(args);
+ }
+ catch (Args.ParseException e) {
+ System.out.println(e.getMessage());
+ System.exit(1);
+ }
+
+ String jsonCfgStr = null;
+ if (parsedArgs.config != null)
+ jsonCfgStr = Files.readString(parsedArgs.config);
+
+ var ignition = new IgnitionImpl();
+
+ ignition.start(parsedArgs.nodeName, jsonCfgStr);
+ }
+
+ /**
+ * Simple value object with parsed CLI args of ignite runner.
+ */
+ private static class Args {
+ /** CLI usage message. */
+ private static String usage = "IgniteCliRunner [--config conf]
nodeName";
+
+ /** Name of the node. */
+ private final String nodeName;
+
+ /** Path to config file. */
+ private final Path config;
+
+ /**
+ * Creates new instance with parsed arguments.
+ *
+ * @param nodeName Name of the node.
+ * @param config Path to config file.
+ */
+ private Args(String nodeName, Path config) {
+ this.nodeName = nodeName;
+ this.config = config;
+ }
+
+ /**
+ * Simple CLI arguments parser.
Review comment:
Should we reflect in javadoc here or somewhere else, how many arguments
we are waiting for and which arguments?
##########
File path:
modules/runner/src/main/java/org/apache/ignite/app/IgniteCliRunner.java
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.app;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.ignite.internal.app.IgnitionImpl;
+
+/**
+ * The main entry point for run new Ignite node from CLI toolchain.
+ */
+public class IgniteCliRunner {
+ /**
+ * Main method for run new Ignite node.
+ *
+ * @param args CLI args to start new node.
+ * @throws IOException if any issues with reading config file.
+ */
+ public static void main(String[] args) throws IOException {
+ Args parsedArgs = null;
+
+ try {
+ parsedArgs = Args.parseArgs(args);
+ }
+ catch (Args.ParseException e) {
+ System.out.println(e.getMessage());
+ System.exit(1);
Review comment:
empty line. Also, should we write here some clarification what happened
before `e.getMessage()`?
##########
File path:
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/IgnitionTest.java
##########
@@ -87,7 +85,7 @@ void testNodesStartWithBootstrapConfiguration() {
@Test
@Disabled("https://issues.apache.org/jira/browse/IGNITE-14709";)
void testNodeStartWithoutBootstrapConfiguration() {
- Ignite ignite = IgnitionManager.start(null);
+ Ignite ignite = IgnitionManager.start(null, null);
Review comment:
what we expect when we start node without bootstarping configuration and
with null as name? As far as I can understand, we'll fall with assert, looks
strange
##########
File path:
modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java
##########
@@ -171,6 +175,31 @@ public boolean bootstrapped() {
}
}
+ /**
+ * Persist node name to the vault.
+ *
+ * @param name Node name to persist.
+ * @return Future representing pending completion of the operation.
Couldn't be {@code null}.
+ */
+ @NotNull public CompletableFuture<Void> putName(@NotNull String name) {
+ return put(NODE_NAME, name.getBytes(StandardCharsets.UTF_8));
+ }
+
+ /**
+ * @return Node name, if was stored earlier.
+ * @throws IgniteInternalCheckedException If couldn't get node name from
the vault.
+ */
+ public String name() throws IgniteInternalCheckedException {
+ synchronized (mux) {
+ try {
+ return new String(vaultService.get(NODE_NAME).get().value());
Review comment:
I'm not sure, but should we consider some caching here? Seems that
`name()` might be called frequently, WDYT?
##########
File path:
modules/runner/src/main/java/org/apache/ignite/internal/app/IgnitionImpl.java
##########
@@ -80,11 +80,14 @@
private static final String VER_KEY = "version";
/** {@inheritDoc} */
- @Override public synchronized Ignite start(String jsonStrBootstrapCfg) {
+ @Override public synchronized Ignite start(String nodeName, String
jsonStrBootstrapCfg) {
+ assert !StringUtil.isNullOrEmpty(nodeName) : "Node local name is
empty";
Review comment:
I would add @NotNull for nodeName here and in ancestor, and also reflect
that facts in javadocs
##########
File path:
modules/runner/src/main/java/org/apache/ignite/app/IgniteCliRunner.java
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.app;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.ignite.internal.app.IgnitionImpl;
+
+/**
+ * The main entry point for run new Ignite node from CLI toolchain.
+ */
+public class IgniteCliRunner {
+ /**
+ * Main method for run new Ignite node.
+ *
+ * @param args CLI args to start new node.
+ * @throws IOException if any issues with reading config file.
+ */
+ public static void main(String[] args) throws IOException {
+ Args parsedArgs = null;
+
+ try {
+ parsedArgs = Args.parseArgs(args);
+ }
+ catch (Args.ParseException e) {
+ System.out.println(e.getMessage());
+ System.exit(1);
+ }
+
+ String jsonCfgStr = null;
+ if (parsedArgs.config != null)
Review comment:
empty line
##########
File path:
modules/runner/src/main/java/org/apache/ignite/app/IgniteCliRunner.java
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.app;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.ignite.internal.app.IgnitionImpl;
+
+/**
+ * The main entry point for run new Ignite node from CLI toolchain.
+ */
+public class IgniteCliRunner {
+ /**
+ * Main method for run new Ignite node.
+ *
+ * @param args CLI args to start new node.
+ * @throws IOException if any issues with reading config file.
+ */
+ public static void main(String[] args) throws IOException {
+ Args parsedArgs = null;
+
+ try {
+ parsedArgs = Args.parseArgs(args);
+ }
+ catch (Args.ParseException e) {
+ System.out.println(e.getMessage());
+ System.exit(1);
+ }
+
+ String jsonCfgStr = null;
+ if (parsedArgs.config != null)
+ jsonCfgStr = Files.readString(parsedArgs.config);
+
+ var ignition = new IgnitionImpl();
+
+ ignition.start(parsedArgs.nodeName, jsonCfgStr);
+ }
+
+ /**
+ * Simple value object with parsed CLI args of ignite runner.
+ */
+ private static class Args {
+ /** CLI usage message. */
+ private static String usage = "IgniteCliRunner [--config conf]
nodeName";
+
+ /** Name of the node. */
+ private final String nodeName;
+
+ /** Path to config file. */
+ private final Path config;
+
+ /**
+ * Creates new instance with parsed arguments.
+ *
+ * @param nodeName Name of the node.
+ * @param config Path to config file.
+ */
+ private Args(String nodeName, Path config) {
+ this.nodeName = nodeName;
+ this.config = config;
+ }
+
+ /**
+ * Simple CLI arguments parser.
+ *
+ * @param args CLI arguments.
+ * @return Parsed arguments.
+ * @throws ParseException if required args are absent.
+ */
+ private static Args parseArgs(String[] args) throws ParseException {
+ if (args.length == 1)
+ 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]));
Review comment:
Should there be some validation? `Path.of` could throw at least
`IllegalArgumentException` or `FileSystemNotFoundException`, should we catch
such cases and rethrow `ParseException`?
##########
File path:
modules/runner/src/main/java/org/apache/ignite/app/IgniteCliRunner.java
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.app;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import org.apache.ignite.internal.app.IgnitionImpl;
+
+/**
+ * The main entry point for run new Ignite node from CLI toolchain.
+ */
+public class IgniteCliRunner {
+ /**
+ * Main method for run new Ignite node.
+ *
+ * @param args CLI args to start new node.
+ * @throws IOException if any issues with reading config file.
+ */
+ public static void main(String[] args) throws IOException {
+ Args parsedArgs = null;
+
+ try {
+ parsedArgs = Args.parseArgs(args);
+ }
+ catch (Args.ParseException e) {
+ System.out.println(e.getMessage());
+ System.exit(1);
+ }
+
+ String jsonCfgStr = null;
+ if (parsedArgs.config != null)
+ jsonCfgStr = Files.readString(parsedArgs.config);
+
+ var ignition = new IgnitionImpl();
+
+ ignition.start(parsedArgs.nodeName, jsonCfgStr);
+ }
+
+ /**
+ * Simple value object with parsed CLI args of ignite runner.
+ */
+ private static class Args {
+ /** CLI usage message. */
+ private static String usage = "IgniteCliRunner [--config conf]
nodeName";
+
+ /** Name of the node. */
+ private final String nodeName;
+
+ /** Path to config file. */
+ private final Path config;
+
+ /**
+ * Creates new instance with parsed arguments.
+ *
+ * @param nodeName Name of the node.
+ * @param config Path to config file.
+ */
+ private Args(String nodeName, Path config) {
+ this.nodeName = nodeName;
+ this.config = config;
+ }
+
+ /**
+ * Simple CLI arguments parser.
+ *
+ * @param args CLI arguments.
+ * @return Parsed arguments.
+ * @throws ParseException if required args are absent.
+ */
+ private static Args parseArgs(String[] args) throws ParseException {
+ if (args.length == 1)
+ 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]));
+ else
+ throw new ParseException(usage);
+ }
+ else
+ throw new ParseException(usage);
+ }
+
+ /**
+ * Exception for indicate any problems with parsing CLI args.
+ */
+ private static class ParseException extends Exception {
Review comment:
Should it extends some Ignite related exception?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
