ibessonov commented on code in PR #1929:
URL: https://github.com/apache/ignite-3/pull/1929#discussion_r1172179670
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationManager.java:
##########
@@ -40,6 +31,30 @@ public class ConfigurationManager implements IgniteComponent
{
/** Configuration registry. */
private final ConfigurationRegistry registry;
+ /**
+ * Constructor.
+ *
+ * @param rootKeys Configuration root keys.
+ * @param validators Validators.
+ * @param storage Configuration storage.
+ * @param generator Configuration tree generator.
+ * @throws IllegalArgumentException If the configuration type of the root
keys is not equal to the storage type, or if the schema or its
+ * extensions are not valid.
+ */
+ public ConfigurationManager(
Review Comment:
Is there a reason to leave the old constructor in this class? Seems
confusing now, but I don't know some nuances probably
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationTreeGenerator.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.configuration;
+
+
+import static java.util.function.Function.identity;
+import static java.util.function.Predicate.not;
+import static java.util.stream.Collectors.toList;
+import static java.util.stream.Collectors.toMap;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.collectSchemas;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.internalSchemaExtensions;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.isPolymorphicId;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.polymorphicInstanceId;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.polymorphicSchemaExtensions;
+import static
org.apache.ignite.internal.configuration.util.ConfigurationUtil.schemaFields;
+import static org.apache.ignite.internal.util.CollectionUtils.difference;
+import static org.apache.ignite.internal.util.CollectionUtils.viewReadOnly;
+
+import java.lang.reflect.Field;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.InternalConfiguration;
+import org.apache.ignite.configuration.annotation.PolymorphicConfigInstance;
+import org.apache.ignite.configuration.annotation.PolymorphicId;
+import org.apache.ignite.internal.close.ManuallyCloseable;
+import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
+import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.internal.configuration.util.ConfigurationUtil;
+import org.jetbrains.annotations.Nullable;
+
+/** Schema-aware configuration generator. */
+public class ConfigurationTreeGenerator implements ManuallyCloseable {
+
+ private final Map<String, RootKey<?, ?>> rootKeys;
+
+ @Nullable
+ private ConfigurationAsmGenerator generator = new
ConfigurationAsmGenerator();
+
+ /**
+ * Constructor that takes a collection of root keys. Internal and
polymorphic schema extensions are empty by default.
+ *
+ * @param rootKeys Root keys.
+ */
+ public ConfigurationTreeGenerator(Collection<RootKey<?, ?>> rootKeys) {
+ this(rootKeys, Set.of(), Set.of());
+ }
Review Comment:
Is this for tests? Mark it as `@TestOnly` then
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationFlattener.java:
##########
@@ -136,6 +136,10 @@ public Void doVisitInnerNode(String key, InnerNode
newNode) {
return null;
}
+ if (oldNode == null && newNode == null) {
Review Comment:
Can you please add a comment, explaining how it's possible?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -113,13 +112,13 @@ public static class ThirdConfigurationSchema {
public String strCfg;
}
- private static ConfigurationAsmGenerator cgen = new
ConfigurationAsmGenerator();
+ private static ConfigurationTreeGenerator generator = new
ConfigurationTreeGenerator(List.of(KEY, DefaultsConfiguration.KEY));
private final TestConfigurationStorage storage = new
TestConfigurationStorage(LOCAL);
@AfterAll
- public static void afterAll() {
- cgen = null;
+ public static void afterAll() throws Exception {
Review Comment:
Does it have to throw an exception? This "close" method in generator simply
nullifies the field, there's no way it ends with exception.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/configuration/ItSslConfigurationValidationTest.java:
##########
@@ -34,6 +35,7 @@
* Integration test for checking SSL configuration validation.
*/
@ExtendWith(WorkDirectoryExtension.class)
+@Disabled("https://issues.apache.org/jira/browse/IGNITE-19315";)
Review Comment:
Too bad, I expected us to preserve this function.
Do we prioritize this new feature as the next one that you'll be working on?
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ SuperRoot superRoot = generator.createSuperRoot();
+ SuperRoot copiedSuperRoot = superRoot.copy();
+
+ Config hocon = readHoconFromFile();
+
HoconConverter.hoconSource(hocon.root()).descend(copiedSuperRoot);
+
+ Map<String, Serializable> flattenedUpdatesMap =
createFlattenedUpdatesMap(superRoot, copiedSuperRoot);
+ flattenedUpdatesMap.forEach((key, value) -> {
+ if (value != null) { // filter defaults
+ latest.put(key, value);
+ }
+ });
+
+ return new Data(flattenedUpdatesMap, lastRevision);
+ } finally {
+ lock.writeLock().unlock();
+ }
+ });
+ }
+
+ private Config readHoconFromFile() {
+ checkAndRestoreConfigFile();
+
+ return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
}
@Override
public CompletableFuture<Map<String, ? extends Serializable>>
readAllLatest(String prefix) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- Map<String, Serializable> map = latest.entrySet()
- .stream()
- .filter(entry -> entry.getKey().startsWith(prefix))
- .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
- return CompletableFuture.completedFuture(map);
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
Review Comment:
Again, I don't get it. We read data from the field. Why do we have to do it
in a separate pool? Makes no sense to me, please add a comment or revert this
change.
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ SuperRoot superRoot = generator.createSuperRoot();
+ SuperRoot copiedSuperRoot = superRoot.copy();
+
+ Config hocon = readHoconFromFile();
+
HoconConverter.hoconSource(hocon.root()).descend(copiedSuperRoot);
+
+ Map<String, Serializable> flattenedUpdatesMap =
createFlattenedUpdatesMap(superRoot, copiedSuperRoot);
+ flattenedUpdatesMap.forEach((key, value) -> {
+ if (value != null) { // filter defaults
Review Comment:
```suggestion
if (value != null) { // Filter defaults.
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -175,56 +232,65 @@ public CompletableFuture<Void>
writeConfigurationRevision(long prevRevision, lon
@Override
public void close() {
- IgniteUtils.shutdownAndAwaitTermination(threadPool, 10,
TimeUnit.SECONDS);
-
futureTracker.cancelInFlightFutures();
+
+ IgniteUtils.shutdownAndAwaitTermination(workerThreadPool, 10,
TimeUnit.SECONDS);
+ IgniteUtils.shutdownAndAwaitTermination(notificationsThreadPool, 10,
TimeUnit.SECONDS);
}
- private void saveValues(Map<String, ? extends Serializable> values) {
+ private void saveConfigFile() {
try {
- Files.write(tempConfigPath,
renderHoconString(values).getBytes(StandardCharsets.UTF_8),
- StandardOpenOption.SYNC, StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING);
- Files.move(tempConfigPath, configPath,
StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+ Files.write(
+ tempConfigPath,
+ renderHoconString().getBytes(StandardCharsets.UTF_8),
+ StandardOpenOption.SYNC, StandardOpenOption.CREATE,
StandardOpenOption.TRUNCATE_EXISTING
+ );
+
+ Files.move(
+ tempConfigPath,
+ configPath,
+ StandardCopyOption.ATOMIC_MOVE,
StandardCopyOption.REPLACE_EXISTING
+ );
} catch (IOException e) {
throw new NodeConfigWriteException(
- "Failed to write values " + values + " to config file.",
e);
+ "Failed to write values to config file.", e);
}
}
/**
* Convert provided map to Hocon String representation.
*
- * @param values Values of configuration.
* @return Configuration file string representation in HOCON format.
*/
- private String renderHoconString(Map<String, ? extends Serializable>
values) {
- Map<String, Object> map =
values.entrySet().stream().collect(Collectors.toMap(Entry::getKey, stringEntry
-> {
- Serializable value = stringEntry.getValue();
- if (value.getClass().isArray()) {
- return Arrays.asList((Object[]) value);
- }
- return value;
- }));
- Config other = ConfigFactory.parseMap(map);
- Config newConfig = other.withFallback(parseConfigOptions()).resolve();
+ private String renderHoconString() {
+ // Super root that'll be filled from the storage data.
+ SuperRoot rootNode = generator.createSuperRoot();
+
+ fillFromPrefixMap(rootNode, toPrefixMap(latest));
+
+ Object transformed = rootNode.accept(null, new
ConverterToMapVisitor(false, true));
+
+ ConfigValue conf = ConfigImpl.fromAnyRef(
+ transformed, null
+ );
Review Comment:
Could be a single line I guess
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ SuperRoot superRoot = generator.createSuperRoot();
+ SuperRoot copiedSuperRoot = superRoot.copy();
+
+ Config hocon = readHoconFromFile();
+
HoconConverter.hoconSource(hocon.root()).descend(copiedSuperRoot);
+
+ Map<String, Serializable> flattenedUpdatesMap =
createFlattenedUpdatesMap(superRoot, copiedSuperRoot);
+ flattenedUpdatesMap.forEach((key, value) -> {
+ if (value != null) { // filter defaults
+ latest.put(key, value);
+ }
+ });
+
+ return new Data(flattenedUpdatesMap, lastRevision);
+ } finally {
+ lock.writeLock().unlock();
+ }
+ });
+ }
+
+ private Config readHoconFromFile() {
+ checkAndRestoreConfigFile();
+
+ return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
}
@Override
public CompletableFuture<Map<String, ? extends Serializable>>
readAllLatest(String prefix) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- Map<String, Serializable> map = latest.entrySet()
- .stream()
- .filter(entry -> entry.getKey().startsWith(prefix))
- .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
- return CompletableFuture.completedFuture(map);
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.entrySet()
+ .stream()
+ .filter(entry -> entry.getKey().startsWith(prefix))
+ .collect(toMap(Entry::getKey, Entry::getValue));
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Serializable> readLatest(String key) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- return CompletableFuture.completedFuture(latest.get(key));
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
Review Comment:
Same here. Literally "Map#get"
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorageTest.java:
##########
@@ -17,75 +17,401 @@
package org.apache.ignite.internal.configuration.storage;
-import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.emptyString;
+import static org.hamcrest.Matchers.equalToCompressingWhiteSpace;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.hasValue;
import static org.hamcrest.Matchers.is;
import java.io.IOException;
+import java.io.Serializable;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Set;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.Value;
+import org.apache.ignite.internal.configuration.TestConfigurationChanger;
+import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
import org.apache.ignite.internal.testframework.WorkDirectory;
import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
-import org.junit.jupiter.api.Disabled;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
-/**
- * Tests for the {@link LocalFileConfigurationStorage}.
- */
+/** Test for local file configurations storage. */
@ExtendWith(WorkDirectoryExtension.class)
-@Disabled("https://issues.apache.org/jira/browse/IGNITE-19152";)
-public class LocalFileConfigurationStorageTest extends
ConfigurationStorageTest {
+public class LocalFileConfigurationStorageTest {
private static final String CONFIG_NAME = "ignite-config.conf";
+ private static ConfigurationAsmGenerator cgen;
+
@WorkDirectory
private Path tmpDir;
- @Override
- public ConfigurationStorage getStorage() {
- return new LocalFileConfigurationStorage(getConfigFile());
+ /** Test configuration storage. */
+ private LocalFileConfigurationStorage storage;
+
+ /** Test configuration changer. */
+ private TestConfigurationChanger changer;
+
+ /** Instantiates {@link #cgen}. */
+ @BeforeAll
+ public static void beforeAll() {
+ cgen = new ConfigurationAsmGenerator();
+ }
+
+ /** Nullifies {@link #cgen} to prevent memory leak from having runtime
ClassLoader accessible from GC root. */
+ @AfterAll
+ public static void afterAll() {
+ cgen = null;
+ }
+
+ private Path getConfigFile() {
+ return tmpDir.resolve(CONFIG_NAME);
+ }
+
+ @BeforeEach
+ void before() {
+ storage = new LocalFileConfigurationStorage(getConfigFile(),
List.of(TopConfiguration.KEY));
+
+ changer = new TestConfigurationChanger(
+ cgen,
+ List.of(TopConfiguration.KEY),
+ Set.of(),
+ storage,
+ List.of(),
+ List.of()
+ );
+
+ changer.start();
+ }
+
+ @AfterEach
+ void after() {
+ changer.stop();
}
@Test
- void testHocon() throws IOException {
- // All of this is needed because write expects serializable values and
only concrete classes are serializable
- HashMap<String, ArrayList<String>> map = new HashMap<>(Map.of("list",
new ArrayList<>(List.of("val1", "val2"))));
- var data = Map.of("foo1", "bar1", "foo2", "bar2", "map", map);
-
- assertThat(storage.write(data, 0), willBe(true));
-
- String contents = Files.readString(getConfigFile());
-
- // \n instead of System.lineSeparator because Config library writes \n
only
- assertThat(contents, is("foo1=bar1\n"
- + "foo2=bar2\n"
- + "map {\n"
- + " list=[\n"
- + " val1,\n"
- + " val2\n"
- + " ]\n"
- + "}\n"));
+ @DisplayName("Default values are not added enriched on read when the
config file is empty")
+ void empty() throws IOException {
+ // Given
+ assertThat(configFileContent(), emptyString());
+
+ // When
+ var storageValues = readAllLatest();
+
+ // Then storage data only contains top level defaults
+ assertThat(storageValues.entrySet(), hasSize(1));
}
@Test
- void testMergeHocon() throws IOException {
- var data = Map.of("foo1", "bar");
- assertThat(storage.write(data, 0), willBe(true));
+ @DisplayName("Named list entities can be added")
+ void add() throws Exception {
+ // Given
+ assertThat(configFileContent(), emptyString());
+ // And
+ var topConfiguration = (TopConfiguration)
cgen.instantiateCfg(TopConfiguration.KEY, changer);
+ topConfiguration.namedList().change(b -> b.create("name1", x -> {
+ x.changeStrVal("strVal1");
+ x.changeIntVal(-1);
+ })).get();
+
+ // When
+ var storageValues = readAllLatest();
- var append = Map.of("foo1", "baz", "foo2", "bar");
- assertThat(storage.write(append, 1), willBe(true));
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(6), hasValue(-1)));
+ assertThat(storageValues, allOf(aMapWithSize(6), hasValue("strVal1")));
+ // And
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " strVal=strVal1\n"
+ + " }\n"
Review Comment:
Just in case - I don't mind custom HOCON renderer. This format is kind-of
trivial, and converting tree to string would only require a few days of coding,
while there are multiple advantages, like the ability to inset comments or to
preserve properties order.
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ SuperRoot superRoot = generator.createSuperRoot();
+ SuperRoot copiedSuperRoot = superRoot.copy();
+
+ Config hocon = readHoconFromFile();
+
HoconConverter.hoconSource(hocon.root()).descend(copiedSuperRoot);
+
+ Map<String, Serializable> flattenedUpdatesMap =
createFlattenedUpdatesMap(superRoot, copiedSuperRoot);
+ flattenedUpdatesMap.forEach((key, value) -> {
+ if (value != null) { // filter defaults
+ latest.put(key, value);
+ }
+ });
+
+ return new Data(flattenedUpdatesMap, lastRevision);
+ } finally {
+ lock.writeLock().unlock();
+ }
+ });
+ }
+
+ private Config readHoconFromFile() {
+ checkAndRestoreConfigFile();
+
+ return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
}
@Override
public CompletableFuture<Map<String, ? extends Serializable>>
readAllLatest(String prefix) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- Map<String, Serializable> map = latest.entrySet()
- .stream()
- .filter(entry -> entry.getKey().startsWith(prefix))
- .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
- return CompletableFuture.completedFuture(map);
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.entrySet()
+ .stream()
+ .filter(entry -> entry.getKey().startsWith(prefix))
+ .collect(toMap(Entry::getKey, Entry::getValue));
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Serializable> readLatest(String key) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- return CompletableFuture.completedFuture(latest.get(key));
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.get(key);
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Boolean> write(Map<String, ? extends
Serializable> newValues, long ver) {
- lock.writeLock().lock();
- try {
- if (ver != lastRevision) {
- return CompletableFuture.completedFuture(false);
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ if (ver != lastRevision) {
+ return false;
+ }
+
+ mergeAndSave(newValues);
+
+ sendNotificationAsync(new Data(newValues, lastRevision));
+
+ return true;
+ } finally {
+ lock.writeLock().unlock();
}
- checkAndRestoreConfigFile();
- // TODO: https://issues.apache.org/jira/browse/IGNITE-19152
- //saveValues(newValues);
- latest.putAll(newValues);
- lastRevision++;
- runAsync(() -> lsnrRef.get().onEntriesChanged(new Data(newValues,
lastRevision)));
- return CompletableFuture.completedFuture(true);
- } finally {
- lock.writeLock().unlock();
- }
+ });
}
- private void runAsync(Runnable runnable) {
- CompletableFuture<Void> future = CompletableFuture.runAsync(runnable,
threadPool);
+ private void mergeAndSave(Map<String, ? extends Serializable> newValues) {
+ updateLatestState(newValues);
+ saveConfigFile();
+ lastRevision++;
+ }
- futureTracker.registerFuture(future);
+ private void updateLatestState(Map<String, ? extends Serializable>
newValues) {
+ newValues.forEach((key, value) -> {
+ if (value == null) { // null means that we should remove this entry
Review Comment:
Please don't ignore coding conventions :)
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ SuperRoot superRoot = generator.createSuperRoot();
+ SuperRoot copiedSuperRoot = superRoot.copy();
+
+ Config hocon = readHoconFromFile();
+
HoconConverter.hoconSource(hocon.root()).descend(copiedSuperRoot);
+
+ Map<String, Serializable> flattenedUpdatesMap =
createFlattenedUpdatesMap(superRoot, copiedSuperRoot);
+ flattenedUpdatesMap.forEach((key, value) -> {
+ if (value != null) { // filter defaults
+ latest.put(key, value);
+ }
+ });
+
+ return new Data(flattenedUpdatesMap, lastRevision);
+ } finally {
+ lock.writeLock().unlock();
+ }
+ });
+ }
+
+ private Config readHoconFromFile() {
+ checkAndRestoreConfigFile();
+
+ return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
}
@Override
public CompletableFuture<Map<String, ? extends Serializable>>
readAllLatest(String prefix) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- Map<String, Serializable> map = latest.entrySet()
- .stream()
- .filter(entry -> entry.getKey().startsWith(prefix))
- .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
- return CompletableFuture.completedFuture(map);
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.entrySet()
+ .stream()
+ .filter(entry -> entry.getKey().startsWith(prefix))
+ .collect(toMap(Entry::getKey, Entry::getValue));
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Serializable> readLatest(String key) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- return CompletableFuture.completedFuture(latest.get(key));
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.get(key);
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Boolean> write(Map<String, ? extends
Serializable> newValues, long ver) {
- lock.writeLock().lock();
- try {
- if (ver != lastRevision) {
- return CompletableFuture.completedFuture(false);
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ if (ver != lastRevision) {
+ return false;
+ }
+
+ mergeAndSave(newValues);
+
+ sendNotificationAsync(new Data(newValues, lastRevision));
+
+ return true;
+ } finally {
+ lock.writeLock().unlock();
}
- checkAndRestoreConfigFile();
- // TODO: https://issues.apache.org/jira/browse/IGNITE-19152
- //saveValues(newValues);
- latest.putAll(newValues);
- lastRevision++;
- runAsync(() -> lsnrRef.get().onEntriesChanged(new Data(newValues,
lastRevision)));
- return CompletableFuture.completedFuture(true);
- } finally {
- lock.writeLock().unlock();
- }
+ });
}
- private void runAsync(Runnable runnable) {
- CompletableFuture<Void> future = CompletableFuture.runAsync(runnable,
threadPool);
+ private void mergeAndSave(Map<String, ? extends Serializable> newValues) {
+ updateLatestState(newValues);
+ saveConfigFile();
+ lastRevision++;
Review Comment:
New revision has been given to you as a parameter of "write" method. Why do
you ignore it and increment the old revision instead? Please don't do that.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java:
##########
@@ -96,8 +83,11 @@ public class ConfigurationRegistry implements
IgniteComponent, ConfigurationStor
/** Configuration change handler. */
private final ConfigurationChanger changer;
- /** Configuration generator. */
- private final ConfigurationAsmGenerator cgen = new
ConfigurationAsmGenerator();
+ /** Runtime implementations generator for node classes. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Flag that indicates if the {@link ConfigurationTreeGenerator} instance
is owned by this object or not. */
+ private boolean ownConfigTreeGenerator = false;
Review Comment:
Why does it matter?
I believe that generator can always be a constructor parameter, and another
component, that instantiated it, should also close it in the end
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
Review Comment:
What's the deal with doing it asynchronously? It's a node start routine, we
can do it in current thread I guess?
Please comment
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -113,13 +112,13 @@ public static class ThirdConfigurationSchema {
public String strCfg;
}
- private static ConfigurationAsmGenerator cgen = new
ConfigurationAsmGenerator();
+ private static ConfigurationTreeGenerator generator = new
ConfigurationTreeGenerator(List.of(KEY, DefaultsConfiguration.KEY));
Review Comment:
Maybe this test-only constructor should use vararg, for convenience
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorage.java:
##########
@@ -56,99 +66,146 @@
public class LocalFileConfigurationStorage implements ConfigurationStorage {
private static final IgniteLogger LOG =
Loggers.forClass(LocalFileConfigurationStorage.class);
- /**
- * Path to config file.
- */
+ /** Path to config file. */
private final Path configPath;
- /**
- * Path to temporary configuration storage.
- */
+ /** Path to temporary configuration storage. */
private final Path tempConfigPath;
+ /** R/W lock to guard the latest configuration and config file. */
private final ReadWriteLock lock = new ReentrantReadWriteLock();
- /**
- * Latest state of last applied configuration.
- */
+ /** Latest state of last applied configuration. */
private final Map<String, Serializable> latest = new ConcurrentHashMap<>();
- /**
- * Configuration changes listener.
- * */
+ /** Configuration tree generator. */
+ private final ConfigurationTreeGenerator generator;
+
+ /** Configuration changes listener. */
private final AtomicReference<ConfigurationStorageListener> lsnrRef = new
AtomicReference<>();
- private final ExecutorService threadPool = Executors.newFixedThreadPool(2,
new NamedThreadFactory("loc-cfg-file", LOG));
+ /** Thread pool for configuration updates notifications. */
+ private final ExecutorService notificationsThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file", LOG)
+ );
+
+ /** Thread pool for configuration updates. */
+ private final ExecutorService workerThreadPool =
Executors.newFixedThreadPool(
+ 2, new NamedThreadFactory("cfg-file-worker", LOG)
+ );
+ /** Tracks all running futures. */
private final InFlightFutures futureTracker = new InFlightFutures();
+ /** Last revision for configuration. */
private long lastRevision = 0L;
/**
* Constructor.
*
* @param configPath Path to node bootstrap configuration file.
+ * @param generator Configuration tree generator.
*/
- public LocalFileConfigurationStorage(Path configPath) {
+ public LocalFileConfigurationStorage(Path configPath,
ConfigurationTreeGenerator generator) {
this.configPath = configPath;
- tempConfigPath = configPath.resolveSibling(configPath.getFileName() +
".tmp");
+ this.generator = generator;
+ this.tempConfigPath =
configPath.resolveSibling(configPath.getFileName() + ".tmp");
+
checkAndRestoreConfigFile();
}
@Override
public CompletableFuture<Data> readDataOnRecovery() {
- return CompletableFuture.completedFuture(new
Data(Collections.emptyMap(), 0));
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ SuperRoot superRoot = generator.createSuperRoot();
+ SuperRoot copiedSuperRoot = superRoot.copy();
+
+ Config hocon = readHoconFromFile();
+
HoconConverter.hoconSource(hocon.root()).descend(copiedSuperRoot);
+
+ Map<String, Serializable> flattenedUpdatesMap =
createFlattenedUpdatesMap(superRoot, copiedSuperRoot);
+ flattenedUpdatesMap.forEach((key, value) -> {
+ if (value != null) { // filter defaults
+ latest.put(key, value);
+ }
+ });
+
+ return new Data(flattenedUpdatesMap, lastRevision);
+ } finally {
+ lock.writeLock().unlock();
+ }
+ });
+ }
+
+ private Config readHoconFromFile() {
+ checkAndRestoreConfigFile();
+
+ return ConfigFactory.parseFile(configPath.toFile(),
ConfigParseOptions.defaults().setAllowMissing(false));
}
@Override
public CompletableFuture<Map<String, ? extends Serializable>>
readAllLatest(String prefix) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- Map<String, Serializable> map = latest.entrySet()
- .stream()
- .filter(entry -> entry.getKey().startsWith(prefix))
- .collect(Collectors.toMap(Entry::getKey, Entry::getValue));
- return CompletableFuture.completedFuture(map);
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.entrySet()
+ .stream()
+ .filter(entry -> entry.getKey().startsWith(prefix))
+ .collect(toMap(Entry::getKey, Entry::getValue));
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Serializable> readLatest(String key) {
- lock.readLock().lock();
- try {
- checkAndRestoreConfigFile();
- return CompletableFuture.completedFuture(latest.get(key));
- } finally {
- lock.readLock().unlock();
- }
+ return async(() -> {
+ lock.readLock().lock();
+ try {
+ return latest.get(key);
+ } finally {
+ lock.readLock().unlock();
+ }
+ });
}
@Override
public CompletableFuture<Boolean> write(Map<String, ? extends
Serializable> newValues, long ver) {
- lock.writeLock().lock();
- try {
- if (ver != lastRevision) {
- return CompletableFuture.completedFuture(false);
+ return async(() -> {
+ lock.writeLock().lock();
+ try {
+ if (ver != lastRevision) {
+ return false;
+ }
+
+ mergeAndSave(newValues);
+
+ sendNotificationAsync(new Data(newValues, lastRevision));
+
+ return true;
+ } finally {
+ lock.writeLock().unlock();
}
- checkAndRestoreConfigFile();
- // TODO: https://issues.apache.org/jira/browse/IGNITE-19152
- //saveValues(newValues);
- latest.putAll(newValues);
- lastRevision++;
- runAsync(() -> lsnrRef.get().onEntriesChanged(new Data(newValues,
lastRevision)));
- return CompletableFuture.completedFuture(true);
- } finally {
- lock.writeLock().unlock();
- }
+ });
}
- private void runAsync(Runnable runnable) {
- CompletableFuture<Void> future = CompletableFuture.runAsync(runnable,
threadPool);
+ private void mergeAndSave(Map<String, ? extends Serializable> newValues) {
+ updateLatestState(newValues);
+ saveConfigFile();
+ lastRevision++;
+ }
- futureTracker.registerFuture(future);
+ private void updateLatestState(Map<String, ? extends Serializable>
newValues) {
+ newValues.forEach((key, value) -> {
+ if (value == null) { // null means that we should remove this entry
Review Comment:
```suggestion
if (value == null) { // Null means that we should remove this
entry.
```
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/LocalFileConfigurationStorageTest.java:
##########
@@ -17,75 +17,457 @@
package org.apache.ignite.internal.configuration.storage;
-import static
org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.emptyString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.equalToCompressingWhiteSpace;
+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.hasValue;
import static org.hamcrest.Matchers.is;
import java.io.IOException;
+import java.io.Serializable;
+import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Set;
+import org.apache.ignite.configuration.annotation.Config;
+import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.NamedConfigValue;
+import org.apache.ignite.configuration.annotation.Value;
+import org.apache.ignite.internal.configuration.ConfigurationTreeGenerator;
+import org.apache.ignite.internal.configuration.TestConfigurationChanger;
import org.apache.ignite.internal.testframework.WorkDirectory;
import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
-import org.junit.jupiter.api.Disabled;
+import org.hamcrest.Matchers;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
-/**
- * Tests for the {@link LocalFileConfigurationStorage}.
- */
+/** Test for local file configurations storage. */
@ExtendWith(WorkDirectoryExtension.class)
-@Disabled("https://issues.apache.org/jira/browse/IGNITE-19152";)
-public class LocalFileConfigurationStorageTest extends
ConfigurationStorageTest {
+//TODO: https://issues.apache.org/jira/browse/IGNITE-19303
+public class LocalFileConfigurationStorageTest {
private static final String CONFIG_NAME = "ignite-config.conf";
+ private static ConfigurationTreeGenerator treeGenerator;
+
@WorkDirectory
private Path tmpDir;
- @Override
- public ConfigurationStorage getStorage() {
- return new LocalFileConfigurationStorage(getConfigFile());
+ private LocalFileConfigurationStorage storage;
+
+ private TestConfigurationChanger changer;
+
+ @BeforeAll
+ public static void beforeAll() {
+ treeGenerator = new ConfigurationTreeGenerator(
+ List.of(TopConfiguration.KEY, TopEmptyConfiguration.KEY)
+ );
+ }
+
+ @AfterAll
+ public static void afterAll() throws Exception {
+ treeGenerator.close();
+ }
+
+ private Path getConfigFile() {
+ return tmpDir.resolve(CONFIG_NAME);
+ }
+
+ @BeforeEach
+ void before() {
+ storage = new LocalFileConfigurationStorage(getConfigFile(),
treeGenerator);
+
+ changer = new TestConfigurationChanger(
+ List.of(TopConfiguration.KEY),
+ Set.of(),
+ storage,
+ treeGenerator
+ );
+
+ changer.start();
+ }
+
+ @AfterEach
+ void after() {
+ changer.stop();
+ }
+
+
+ /** Default values are not enriched on read when the config file is empty.
*/
+ @Test
+ void empty() throws IOException {
+ // Given
+ assertThat(configFileContent(), emptyString());
+
+ // When
+ var storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues.entrySet(), hasSize(0));
+ }
+
+ /** Named list entities can be added. */
+ @Test
+ void add() throws Exception {
+ // Given
+ assertThat(configFileContent(), emptyString());
+
+ // And
+ var topConfiguration = (TopConfiguration)
treeGenerator.instantiateCfg(TopConfiguration.KEY, changer);
+
+ topConfiguration.namedList().change(b -> b.create("name1", x -> {
+ x.changeStrVal("strVal1");
+ x.changeIntVal(-1);
+ })).get();
+
+ // When
+ var storageValues = readAllLatest();
+
+ // Then the map has updated values
+ //
+ // top.namedList.<generatedUUID>.strVal -> strVal1
+ // top.namedList.<generatedUUID>.intVal -> -1
+ // top.namedList.<generatedUUID>.<name> -> name1
+ // top.namedList.<ids>.name1 -> "<generatedUUID>"
+ // top.namedList.<generatedUUID>.<order> -> 0
+
+ assertThat(storageValues, allOf(aMapWithSize(5), hasValue(-1)));
+ assertThat(storageValues, allOf(aMapWithSize(5), hasValue("strVal1")));
+
+ // And
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " strVal=strVal1\n"
+ + " }\n"
+ + " ]\n"
+ + "}"
+ ));
+
+ // When
+ topConfiguration.namedList().change(b -> b.create("name2", x -> {
+ x.changeStrVal("strVal2");
+ x.changeIntVal(-2);
+ })).get();
+ // And
+ storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(10), hasValue(-2)));
+ assertThat(storageValues, allOf(aMapWithSize(10),
hasValue("strVal2")));
+ // And
+ assertThat(storageValues, allOf(aMapWithSize(10), hasValue(-1)));
+ assertThat(storageValues, allOf(aMapWithSize(10),
hasValue("strVal1")));
+ // And
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " strVal=strVal1\n"
+ + " },\n"
+ + " {\n"
+ + " intVal=-2\n"
+ + " name=name2\n"
+ + " strVal=strVal2\n"
+ + " }\n"
+ + " ]\n"
+ + "}\n"
+ ));
+ }
+
+ /** Update values. */
+ @Test
+ void update() throws Exception {
+ // Given
+ assertThat(configFileContent(), emptyString());
+
+ // When
+ var topConfiguration = (TopConfiguration)
treeGenerator.instantiateCfg(TopConfiguration.KEY, changer);
+
+ topConfiguration.shortVal().update((short) 3).get();
+ // And
+ var storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(1), hasValue((short) 3)));
+ // And
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " shortVal=3\n"
+ + "}\n"
+ ));
+
+ // When create named list entity with defaults
+ topConfiguration.namedList().change(b -> b.create("name1", x -> {
+ })).get();
+ // And
+ storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(6), hasValue(1)));
+ assertThat(storageValues, allOf(aMapWithSize(6), hasValue("foo")));
+ // And
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=1\n"
+ + " name=name1\n"
+ + " strVal=foo\n"
+ + " }\n"
+ + " ]\n"
+ + " shortVal=3\n"
+ + "}"
+ ));
+
+ // When update named list entity
+ topConfiguration.namedList().change(b -> b.update("name1", x -> {
+ x.changeStrVal("strVal1");
+ x.changeIntVal(-1);
+ })).get();
+ // And
+ storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(6), hasValue(-1)));
+ assertThat(storageValues, allOf(aMapWithSize(6), hasValue("strVal1")));
+ // And
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " strVal=strVal1\n"
+ + " }\n"
+ + " ]\n"
+ + " shortVal=3\n"
+ + "}"
+ ));
}
+ /** Remove values. */
@Test
- void testHocon() throws IOException {
- // All of this is needed because write expects serializable values and
only concrete classes are serializable
- HashMap<String, ArrayList<String>> map = new HashMap<>(Map.of("list",
new ArrayList<>(List.of("val1", "val2"))));
- var data = Map.of("foo1", "bar1", "foo2", "bar2", "map", map);
-
- assertThat(storage.write(data, 0), willBe(true));
-
- String contents = Files.readString(getConfigFile());
-
- // \n instead of System.lineSeparator because Config library writes \n
only
- assertThat(contents, is("foo1=bar1\n"
- + "foo2=bar2\n"
- + "map {\n"
- + " list=[\n"
- + " val1,\n"
- + " val2\n"
+ void remove() throws Exception {
+ // Given
+ var topConfiguration = (TopConfiguration)
treeGenerator.instantiateCfg(TopConfiguration.KEY, changer);
+
+ topConfiguration.namedList().change(b -> {
+ b.create("name1", x -> {
+ x.changeStrVal("strVal1");
+ x.changeIntVal(-1);
+ });
+ b.create("name2", x -> {
+ x.changeStrVal("strVal2");
+ x.changeIntVal(-2);
+ });
+ }).get();
+
+ topConfiguration.shortVal().update((short) 3).get();
+ // And values are saved to file
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " strVal=strVal1\n"
+ + " },\n"
+ + " {\n"
+ + " intVal=-2\n"
+ + " name=name2\n"
+ + " strVal=strVal2\n"
+ + " }\n"
+ + " ]\n"
+ + " shortVal=3\n"
+ + "}\n"
+ ));
+
+ // When remove named list entity
+ topConfiguration.namedList().change(b -> b.delete("name1")).get();
+ // And
+ var storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(6),
Matchers.not(hasValue("strVal1"))));
+ // And entity removed from file
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-2\n"
+ + " name=name2\n"
+ + " strVal=strVal2\n"
+ + " }\n"
+ + " ]\n"
+ + " shortVal=3\n"
+ + "}\n"
+ ));
+
+ // When remove the last entity
+ topConfiguration.namedList().change(b -> b.delete("name2")).get();
+ // And
+ storageValues = readAllLatest();
+
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(1), hasValue((short) 3)));
+ // And entity removed from file
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " shortVal=3\n"
+ + "}\n"
+ ));
+ }
+
+ /** Delete file before read on recovery. */
+ @Test
+ void deleteFileBeforeReadOnRecovery() throws IOException {
+ // Given
+ Files.delete(getConfigFile());
+
+ // When
+ var storageValues = storage.readDataOnRecovery().join().values();
+
+ // Then
+ assertThat(storageValues.entrySet(), hasSize(0));
+ // And empty file was created
+ assertThat(configFileContent(), equalTo(""));
+ }
+
+
+ /** File content is not changed when read data on recovery. */
+ @Test
+ void fileContentIsNotChanged() throws IOException {
+ // Given
+ String fileContent = "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " }\n"
+ " ]\n"
- + "}\n"));
+ + "}\n";
+
+ Files.write(getConfigFile(),
fileContent.getBytes(StandardCharsets.UTF_8));
+
+ // When
+ var storageValues = storage.readDataOnRecovery().join().values();
+ // Then
+ assertThat(storageValues, allOf(aMapWithSize(5), hasValue(-1)));
+ assertThat(storageValues, allOf(aMapWithSize(5), hasValue("foo"))); //
default value
+ // And file was not changed
+ assertThat(configFileContent(), equalTo(fileContent));
}
+ /** Delete file before read all. */
@Test
- void testMergeHocon() throws IOException {
- var data = Map.of("foo1", "bar");
- assertThat(storage.write(data, 0), willBe(true));
+ void deleteFileBeforeReadAll() throws Exception {
+ // Given
+ Files.delete(getConfigFile());
+
+ // When
+ var storageValues = readAllLatest();
- var append = Map.of("foo1", "baz", "foo2", "bar");
- assertThat(storage.write(append, 1), willBe(true));
+ // Then
+ assertThat(storageValues.entrySet(), hasSize(0));
+ // And there is no file
+ assertThat(Files.exists(getConfigFile()), is(false));
- String contents = Files.readString(getConfigFile());
- assertThat(contents, is("foo1=baz\n"
- + "foo2=bar\n"));
+ // When update configuration
+ var topConfiguration = (TopConfiguration)
treeGenerator.instantiateCfg(TopConfiguration.KEY, changer);
+ topConfiguration.namedList().change(b -> b.create("name1", x -> {
+ x.changeStrVal("strVal1");
+ x.changeIntVal(-1);
+ })).get();
+
+ // Then file is created
+ assertThat(configFileContent(), equalToCompressingWhiteSpace(
+ "top {\n"
+ + " namedList=[\n"
+ + " {\n"
+ + " intVal=-1\n"
+ + " name=name1\n"
+ + " strVal=strVal1\n"
+ + " }\n"
+ + " ]\n"
+ + "}\n"
+ ));
}
- private Path getConfigFile() {
- return tmpDir.resolve(CONFIG_NAME);
+ /** Read configuration when inner node configured with partial content
(some fields are empty). */
+ @Test
+ void innerNodeWithPartialContent() throws Exception {
+ String content = "top: { inner.boolVal: true }";
+ Files.write(getConfigFile(), content.getBytes(StandardCharsets.UTF_8));
+
+ storage.readDataOnRecovery().get();
Review Comment:
Should we assert the size of the map?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
