[GitHub] [ignite-3] ibessonov commented on a change in pull request #256: IGNITE-14956 Implement the VaultManager#bootstrapped method

[GitBox <gi...@apache.org>]

ibessonov commented on a change in pull request #256:
URL: https://github.com/apache/ignite-3/pull/256#discussion_r683165207



##########
File path: modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/AbstractExecutionTest.java
##########
@@ -17,12 +17,11 @@
 
 package org.apache.ignite.internal.processors.query.calcite.exec.rel;
 
+import com.google.common.collect.ImmutableMap;

Review comment:
       Wrong imports reordering

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
##########
@@ -487,4 +496,43 @@ public static void closeAll(Collection<? extends AutoCloseable> closeables) thro
     public static void closeAll(AutoCloseable... closeables) throws Exception {
         closeAll(Arrays.asList(closeables));
     }
+
+    /**
+     * Short date format pattern for log messages in "quiet" mode.
+     * Only time is included since we don't expect "quiet" mode to be used
+     * for longer runs.
+     */
+    private static final SimpleDateFormat SHORT_DATE_FMT = new SimpleDateFormat("HH:mm:ss");

Review comment:
       First, why not declare it as DateFormat instead of specific class? Usually field type is more general than type of the instance. I notice such pattern regularly in your code.
   Second, SimpleDateFormatter is not threadsafe, you should probably use some DateTimeFormatter instead

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
##########
@@ -487,4 +496,43 @@ public static void closeAll(Collection<? extends AutoCloseable> closeables) thro
     public static void closeAll(AutoCloseable... closeables) throws Exception {
         closeAll(Arrays.asList(closeables));
     }
+
+    /**
+     * Short date format pattern for log messages in "quiet" mode.
+     * Only time is included since we don't expect "quiet" mode to be used
+     * for longer runs.
+     */
+    private static final SimpleDateFormat SHORT_DATE_FMT = new SimpleDateFormat("HH:mm:ss");
+
+    /**
+     * @param log Logger.
+     * @param msg Message.
+     */
+    public static void dumpStack(IgniteLogger log, String msg) {
+        String reason = "Dumping stack.";
+
+        var err = new Exception(msg);
+
+        if (log != null)
+            log.error(compact(reason), err);

Review comment:
       Why do you need to compact the reason if it's a constant string? Something's wrong with this method.

##########
File path: modules/runner/src/main/java/org/apache/ignite/internal/app/IgnitionImpl.java
##########
@@ -220,16 +218,19 @@ private static Ignite doStart(String nodeName, @Nullable String cfgContent, Path
                 new ConfigurationManager(rootKeys, cfgStorages)
             );
 
-            if (!cfgBootstrappedFromPds && cfgContent != null)
+            boolean cfgBootstrappedFromPds = locConfigurationMgr.bootstrapped();
+
+            if (!cfgBootstrappedFromPds && cfgContent != null) {
                 try {
                     locConfigurationMgr.bootstrap(cfgContent, ConfigurationType.LOCAL);
                 }
                 catch (Exception e) {
                     LOG.warn("Unable to parse user-specific configuration, default configuration will be used: {}", e.getMessage());
                 }
+            }
             else if (cfgContent != null)
                 LOG.warn("User specific configuration will be ignored, cause vault was bootstrapped with pds configuration");
-            else

Review comment:
       This condition is excessive, can it be a comment instead of the actual check?

##########
File path: modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/NodeRestartTest.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.internal.runner.app;
+
+import org.apache.ignite.app.Ignite;
+import org.apache.ignite.app.IgnitionManager;
+import org.apache.ignite.internal.schema.configuration.SchemaConfigurationConverter;
+import org.apache.ignite.internal.testframework.IgniteAbstractTest;
+import org.apache.ignite.schema.ColumnType;
+import org.apache.ignite.schema.SchemaBuilders;
+import org.apache.ignite.schema.SchemaTable;
+import org.apache.ignite.table.Table;
+import org.apache.ignite.table.Tuple;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+/**
+ * These tests check node restart scenarios.
+ */
+public class NodeRestartTest extends IgniteAbstractTest {

Review comment:
       Name should be "ITNodeRestartTest"

##########
File path: modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java
##########
@@ -51,6 +53,13 @@
 
     private static final boolean assertionsEnabled;
 
+    /**
+     * Gets the current monotonic time in milliseconds.
+     */
+    public static long monotonicMs() {

Review comment:
       Technically this is not true, nano time can in theory overflow and become negative. Why don't you use curentTimeMillis?

##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -73,6 +73,9 @@
     /** Annotation classes mapped to validator objects. */
     private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
 
+    /** The flag is true when at least one value was read from a storage, otherwise false. */
+    private volatile boolean bootstrapped;

Review comment:
       Why do you think that this is a good idea to put "bootstrapped" flag into configuration changer? Shouldn't it be a property of the Vault?




-- 
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