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/08/18 16:01:35 UTC
[GitHub] [ignite-3] SammyVimes opened a new pull request, #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
SammyVimes opened a new pull request, #1023:
URL: https://github.com/apache/ignite-3/pull/1023
https://issues.apache.org/jira/browse/IGNITE-17555
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950146930
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
Review Comment:
But I suppose that some parts of this comment are still valid, aren't they?
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r949996165
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
- // - First update ever, MASTER_KEY property must be absent from MetaStorage.
- // - Current node has already performed some updates or received them from MetaStorage watch listener. In this
- // case "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and received updates from MetaStorage watch listeners after that. Same as
- // above, "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and have not received any updates from MetaStorage watch listeners yet.
- // In this case "curChangeId" matches APPLIED_REV, which may or may not match the MASTER_KEY revision. Two
- // options here:
- // - MASTER_KEY is missing in local MetaStorage copy. This means that current node have not performed nor
- // observed any configuration changes. Valid condition is "MASTER_KEY does not exist".
- // - MASTER_KEY is present in local MetaStorage copy. The MASTER_KEY revision is unknown but is less than or
- // equal to APPLIED_REV. Obviously, there have been no updates from the future yet. It's also guaranteed
- // that the next received configuration update will have the MASTER_KEY revision strictly greater than
- // current APPLIED_REV. This allows to conclude that "MASTER_KEY revision <= curChangeId" is a valid
- // condition for update.
- // Combining all of the above, it's concluded that the following condition must be used:
SimpleCondition condition = curChangeId == 0L
? Conditions.notExists(MASTER_KEY)
: Conditions.revision(MASTER_KEY).le(curChangeId);
Review Comment:
oh, right
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950137530
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ByteUtils.java:
##########
@@ -86,7 +86,7 @@ public static byte[] longToBytes(long l) {
* @param limit Limit of bytes to write into output.
* @return Number of bytes overwritten in {@code bytes} array.
*/
- private static byte[] longToBytes(long l, byte[] bytes, int off, int limit) {
+ public static byte[] longToBytes(long l, byte[] bytes, int off, int limit) {
Review Comment:
You're right
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950052631
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
- // - First update ever, MASTER_KEY property must be absent from MetaStorage.
- // - Current node has already performed some updates or received them from MetaStorage watch listener. In this
- // case "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and received updates from MetaStorage watch listeners after that. Same as
- // above, "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and have not received any updates from MetaStorage watch listeners yet.
- // In this case "curChangeId" matches APPLIED_REV, which may or may not match the MASTER_KEY revision. Two
- // options here:
- // - MASTER_KEY is missing in local MetaStorage copy. This means that current node have not performed nor
- // observed any configuration changes. Valid condition is "MASTER_KEY does not exist".
- // - MASTER_KEY is present in local MetaStorage copy. The MASTER_KEY revision is unknown but is less than or
- // equal to APPLIED_REV. Obviously, there have been no updates from the future yet. It's also guaranteed
- // that the next received configuration update will have the MASTER_KEY revision strictly greater than
- // current APPLIED_REV. This allows to conclude that "MASTER_KEY revision <= curChangeId" is a valid
- // condition for update.
- // Combining all of the above, it's concluded that the following condition must be used:
SimpleCondition condition = curChangeId == 0L
? Conditions.notExists(MASTER_KEY)
: Conditions.revision(MASTER_KEY).le(curChangeId);
Review Comment:
fixed
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950137154
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -27,11 +27,11 @@
*/
public interface ConfigurationStorage extends AutoCloseable {
/**
- * Read all configuration values and current storage version.
+ * Reads all configuration values and current storage version on the start.
Review Comment:
As I understand, it only makes sense on the start (and it's only called on the start). It does some additional stuff like deciding what revision to use
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -27,11 +27,11 @@
*/
public interface ConfigurationStorage extends AutoCloseable {
/**
- * Read all configuration values and current storage version.
+ * Reads all configuration values and current storage version on the start.
Review Comment:
As I understand, it only makes sense on the start (and it's only called on the start). It does some additional stuff like deciding what revision to use, it only makes sense on recovery
--
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] ibessonov commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r949878512
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
- // - First update ever, MASTER_KEY property must be absent from MetaStorage.
- // - Current node has already performed some updates or received them from MetaStorage watch listener. In this
- // case "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and received updates from MetaStorage watch listeners after that. Same as
- // above, "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and have not received any updates from MetaStorage watch listeners yet.
- // In this case "curChangeId" matches APPLIED_REV, which may or may not match the MASTER_KEY revision. Two
- // options here:
- // - MASTER_KEY is missing in local MetaStorage copy. This means that current node have not performed nor
- // observed any configuration changes. Valid condition is "MASTER_KEY does not exist".
- // - MASTER_KEY is present in local MetaStorage copy. The MASTER_KEY revision is unknown but is less than or
- // equal to APPLIED_REV. Obviously, there have been no updates from the future yet. It's also guaranteed
- // that the next received configuration update will have the MASTER_KEY revision strictly greater than
- // current APPLIED_REV. This allows to conclude that "MASTER_KEY revision <= curChangeId" is a valid
- // condition for update.
- // Combining all of the above, it's concluded that the following condition must be used:
SimpleCondition condition = curChangeId == 0L
? Conditions.notExists(MASTER_KEY)
: Conditions.revision(MASTER_KEY).le(curChangeId);
Review Comment:
Should replace that with `eq` probably
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950146465
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -185,38 +190,52 @@ public CompletableFuture<Serializable> readLatest(String key) {
/** {@inheritDoc} */
@Override
- public CompletableFuture<Data> readAll() throws StorageException {
+ public CompletableFuture<Data> readAllOnStart() throws StorageException {
return registerFuture(vaultMgr.get(MetaStorageManager.APPLIED_REV)
- .thenApplyAsync(appliedRevEntry -> {
- long appliedRevision = appliedRevEntry == null ? 0L : ByteUtils.bytesToLong(appliedRevEntry.value());
+ .thenComposeAsync(appliedRevEntry -> {
Review Comment:
Or we can take this even further and split `readAll` into two methods: one to resolve the required revision and another to read the data
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950141625
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -185,38 +190,52 @@ public CompletableFuture<Serializable> readLatest(String key) {
/** {@inheritDoc} */
@Override
- public CompletableFuture<Data> readAll() throws StorageException {
+ public CompletableFuture<Data> readAllOnStart() throws StorageException {
return registerFuture(vaultMgr.get(MetaStorageManager.APPLIED_REV)
- .thenApplyAsync(appliedRevEntry -> {
- long appliedRevision = appliedRevEntry == null ? 0L : ByteUtils.bytesToLong(appliedRevEntry.value());
+ .thenComposeAsync(appliedRevEntry -> {
Review Comment:
I knew you'd be able to suggest something about this mess!
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950140250
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
Review Comment:
The original author of the comment (Ivan) suggested that it is not needed anymore as we have a more strict contract on what the value might be
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950164988
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
Review Comment:
It's a tribute to the original bug I was constantly facing. It's extremely fast, so I feel like it's ok? Or I can change it if it looks cryptic
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950189145
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -27,11 +27,11 @@
*/
public interface ConfigurationStorage extends AutoCloseable {
/**
- * Read all configuration values and current storage version.
+ * Reads all configuration values and current storage version on the start.
Review Comment:
readDataOnRecovery is also fine, I guess
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950229442
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
Review Comment:
Sure
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950280506
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -185,39 +190,54 @@ public CompletableFuture<Serializable> readLatest(String key) {
/** {@inheritDoc} */
@Override
- public CompletableFuture<Data> readAll() throws StorageException {
+ public CompletableFuture<Data> readDataOnRecovery() throws StorageException {
return registerFuture(vaultMgr.get(MetaStorageManager.APPLIED_REV)
- .thenApplyAsync(appliedRevEntry -> {
- long appliedRevision = appliedRevEntry == null ? 0L : ByteUtils.bytesToLong(appliedRevEntry.value());
+ .thenCombineAsync(vaultMgr.get(CONFIGURATION_REVISIONS_KEY), this::readAllOnStart0, threadPool));
+ }
+
+ @NotNull
+ private Data readAllOnStart0(VaultEntry appliedRevEntry, VaultEntry revisionsEntry) {
Review Comment:
1. Parameters should be marked with `@Nullable`
2. We don't use `@NotNull`
--
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] SammyVimes merged pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes merged PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r949968986
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/notifications/ConfigurationNotifier.java:
##########
@@ -425,9 +425,13 @@ public Void visitNamedListNode(String key, NamedListNode<?> newNamedList) {
// Lazy initialization.
Collection<DynamicConfiguration<InnerNode, ?>> newAnyConfigs = null;
+ NamedListConfiguration<?, InnerNode, ?> namedListCfg = namedDynamicConfig(config, key);
+
+ namedListCfg.touchMembers();
Review Comment:
Please add a comment about why we need to "touch members"
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -27,11 +27,11 @@
*/
public interface ConfigurationStorage extends AutoCloseable {
/**
- * Read all configuration values and current storage version.
+ * Reads all configuration values and current storage version on the start.
Review Comment:
I don't understand why the "start" part is so important. What will happen if I call this method not on node start?
##########
modules/metastorage-client/src/main/java/org/apache/ignite/internal/metastorage/client/EntryImpl.java:
##########
@@ -48,7 +48,7 @@ public final class EntryImpl implements Entry {
* @param rev Revision.
* @param updCntr Update counter.
*/
- EntryImpl(@NotNull ByteArray key, @Nullable byte[] val, long rev, long updCntr) {
+ public EntryImpl(@NotNull ByteArray key, @Nullable byte[] val, long rev, long updCntr) {
Review Comment:
Not related to this PR, but can you please fix `Nullable` usages in this class?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -572,20 +572,23 @@ private CompletableFuture<Void> updateFromListener(Data changedEntries) {
storageRoots = new StorageRoots(newSuperRoot, newChangeId);
- if (dataValuesPrefixMap.isEmpty()) {
- oldStorageRoots.changeFuture.complete(null);
-
- return CompletableFuture.completedFuture(null);
- } else {
- return notificator.notify(oldSuperRoot, newSuperRoot, newChangeId, notificationListenerCnt.incrementAndGet())
- .whenComplete((v, t) -> {
- if (t == null) {
- oldStorageRoots.changeFuture.complete(null);
- } else {
- oldStorageRoots.changeFuture.completeExceptionally(t);
- }
- });
- }
+ // Save revisions for recovery.
+ return storage.writeConfigurationRevision(oldStorageRoots.version, storageRoots.version).thenCompose(unused -> {
Review Comment:
Can you move the `thenCompose` call to the next line? I think it makes the code more readable
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
Review Comment:
Why did you remove this comment?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ByteUtils.java:
##########
@@ -86,7 +86,7 @@ public static byte[] longToBytes(long l) {
* @param limit Limit of bytes to write into output.
* @return Number of bytes overwritten in {@code bytes} array.
*/
- private static byte[] longToBytes(long l, byte[] bytes, int off, int limit) {
+ public static byte[] longToBytes(long l, byte[] bytes, int off, int limit) {
Review Comment:
I think that the `limit` parameter makes little sense. It's always equal to 8 anyway
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
+ }
+ }
+
+ /**
+ * This class stores data for {@link MetaStorageManager}'s mock.
+ */
+ private static class MetaStorageMockWrapper {
+ private static final String DISTRIBUTED_PREFIX = "dst-cfg.";
+
+ /**
+ * This and previous field are copy-pasted intentionally, so in case if something changes,
+ * this test should fail and be reviewed and re-wrote.
Review Comment:
```suggestion
* this test should fail and be reviewed and re-written.
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -185,38 +190,52 @@ public CompletableFuture<Serializable> readLatest(String key) {
/** {@inheritDoc} */
@Override
- public CompletableFuture<Data> readAll() throws StorageException {
+ public CompletableFuture<Data> readAllOnStart() throws StorageException {
return registerFuture(vaultMgr.get(MetaStorageManager.APPLIED_REV)
- .thenApplyAsync(appliedRevEntry -> {
- long appliedRevision = appliedRevEntry == null ? 0L : ByteUtils.bytesToLong(appliedRevEntry.value());
+ .thenComposeAsync(appliedRevEntry -> {
Review Comment:
I have several comments here:
1. `thenComposeAsync` is not needed here, since the lambda inside is already an async operation, `thenCompose` should be enough.
2. `thenCompose` enforces serialized reads from the vault, which we don't need. I think that `thenCombine` method is more effective here.
3. The big lambda inside should be extracted into a separate method (e.g. `readAll`).
Taking this into account, we can refactor the method using a following approach:
```
CompletableFuture<Data> future = vaultMgr.get(MetaStorageManager.APPLIED_REV)
.thenCombineAsync(vaultMgr.get(CONFIGURATION_REVISIONS_KEY), this::readAll, threadPool);
return registerFuture(future);
```
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
+ }
+ }
+
+ /**
+ * This class stores data for {@link MetaStorageManager}'s mock.
+ */
+ private static class MetaStorageMockWrapper {
Review Comment:
we now have two mock metastorage managers here. I think this test should be extracted into a separate class
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
+ }
+ }
+
+ /**
+ * This class stores data for {@link MetaStorageManager}'s mock.
+ */
+ private static class MetaStorageMockWrapper {
+ private static final String DISTRIBUTED_PREFIX = "dst-cfg.";
+
+ /**
+ * This and previous field are copy-pasted intentionally, so in case if something changes,
+ * this test should fail and be reviewed and re-wrote.
+ */
+ private static final ByteArray MASTER_KEY = new ByteArray(DISTRIBUTED_PREFIX + "$master$key");
+
+ private static final ByteArray TEST_KEY = new ByteArray(DISTRIBUTED_PREFIX + "someKey.foobar");
+
+ /** MetaStorage mock. */
+ private final MetaStorageManager mock;
+
+ /** Captured MetaStorage listener. */
+ private WatchListener lsnr;
+
+ /** Current master key revision. */
+ private long masterKeyRevision;
+
+ private MetaStorageMockWrapper() {
+ mock = mock(MetaStorageManager.class);
+
+ setup();
+ }
+
+ private void setup() {
+ // Returns current master key revision.
+ when(mock.get(MASTER_KEY)).then(invocation -> {
+ return CompletableFuture.completedFuture(new EntryImpl(MASTER_KEY, null, masterKeyRevision, -1));
+ });
+
+ // On any invocation - trigger storage listener.
+ when(mock.invoke(any(), Mockito.<Collection<Operation>>any(), Mockito.any()))
Review Comment:
can be `when(mock.invoke(any(), anyCollection(), any()))`
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
+ }
+ }
+
+ /**
+ * This class stores data for {@link MetaStorageManager}'s mock.
+ */
+ private static class MetaStorageMockWrapper {
+ private static final String DISTRIBUTED_PREFIX = "dst-cfg.";
+
+ /**
+ * This and previous field are copy-pasted intentionally, so in case if something changes,
+ * this test should fail and be reviewed and re-wrote.
+ */
+ private static final ByteArray MASTER_KEY = new ByteArray(DISTRIBUTED_PREFIX + "$master$key");
+
+ private static final ByteArray TEST_KEY = new ByteArray(DISTRIBUTED_PREFIX + "someKey.foobar");
+
+ /** MetaStorage mock. */
+ private final MetaStorageManager mock;
+
+ /** Captured MetaStorage listener. */
+ private WatchListener lsnr;
+
+ /** Current master key revision. */
+ private long masterKeyRevision;
+
+ private MetaStorageMockWrapper() {
+ mock = mock(MetaStorageManager.class);
+
+ setup();
+ }
+
+ private void setup() {
+ // Returns current master key revision.
+ when(mock.get(MASTER_KEY)).then(invocation -> {
+ return CompletableFuture.completedFuture(new EntryImpl(MASTER_KEY, null, masterKeyRevision, -1));
+ });
+
+ // On any invocation - trigger storage listener.
+ when(mock.invoke(any(), Mockito.<Collection<Operation>>any(), Mockito.any()))
+ .then(invocation -> {
+ triggerStorageListener();
+
+ return CompletableFuture.completedFuture(true);
+ });
+
+ when(mock.invoke(any(), Mockito.<Operation>any(), Mockito.any()))
Review Comment:
can be `when(mock.invoke(any(), any(Operation.class), any()))`
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
Review Comment:
I guess this should be moved to a `finally` block
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
Review Comment:
why 7? Isn't one change enough?
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
Review Comment:
I guess this future can be inlined
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950173974
##########
modules/metastorage-client/src/main/java/org/apache/ignite/internal/metastorage/client/EntryImpl.java:
##########
@@ -48,7 +48,7 @@ public final class EntryImpl implements Entry {
* @param rev Revision.
* @param updCntr Update counter.
*/
- EntryImpl(@NotNull ByteArray key, @Nullable byte[] val, long rev, long updCntr) {
+ public EntryImpl(@NotNull ByteArray key, @Nullable byte[] val, long rev, long updCntr) {
Review Comment:
Sure
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950147140
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
+ }
+ }
+
+ /**
+ * This class stores data for {@link MetaStorageManager}'s mock.
+ */
+ private static class MetaStorageMockWrapper {
Review Comment:
right
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950276651
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationCatchUpTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * 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.storage;
+
+import static org.apache.ignite.configuration.annotation.ConfigurationType.DISTRIBUTED;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyCollection;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Consumer;
+import org.apache.ignite.configuration.RootKey;
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+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.configuration.tree.ConfigurationSource;
+import org.apache.ignite.internal.configuration.tree.ConstructableTreeNode;
+import org.apache.ignite.internal.metastorage.MetaStorageManager;
+import org.apache.ignite.internal.metastorage.client.EntryEvent;
+import org.apache.ignite.internal.metastorage.client.EntryImpl;
+import org.apache.ignite.internal.metastorage.client.Operation;
+import org.apache.ignite.internal.metastorage.client.WatchEvent;
+import org.apache.ignite.internal.metastorage.client.WatchListener;
+import org.apache.ignite.internal.util.ByteUtils;
+import org.apache.ignite.internal.vault.VaultManager;
+import org.apache.ignite.internal.vault.inmemory.InMemoryVaultService;
+import org.apache.ignite.lang.ByteArray;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for the {@link DistributedConfigurationStorage}.
+ */
+public class DistributedConfigurationCatchUpTest {
+ private final VaultManager vaultManager = new VaultManager(new InMemoryVaultService());
+
+ /**
+ * Before each.
+ */
+ @BeforeEach
+ void start() {
+ vaultManager.start();
+ }
+
+ /**
+ * After each.
+ */
+ @AfterEach
+ void stop() throws Exception {
+ vaultManager.stop();
+ }
+
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ // This value is a tribute to the specific error that was occurring. Can as well be 1.
Review Comment:
This comment is as cryptic as the value 7 =)
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950174712
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
Review Comment:
Yes, but only the obvious part, so "we only can change metastorage if didn't change"
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950175168
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -27,11 +27,11 @@
*/
public interface ConfigurationStorage extends AutoCloseable {
/**
- * Read all configuration values and current storage version.
+ * Reads all configuration values and current storage version on the start.
Review Comment:
how about recoveryReadData? Or maybe readDataOnRecovery?
--
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] SammyVimes commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950146164
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ for (int i = 0; i < configurationChangesCount; i++) {
+ ConfigurationSource source = source(
+ rootKey,
+ (DistributedTestChange change) -> change.changeFoobar(1)
+ );
+
+ CompletableFuture<Void> change = changer.change(source);
+
+ change.get();
+ }
+
+ changer.stop();
+ }
+
+ // Put a value to the configuration, so we start on non-empty vault.
+ vaultManager.put(MetaStorageMockWrapper.TEST_KEY, new byte[]{4, 1, 2, 3, 4}).get();
+
+ // This emulates a change in MetaStorage that is not related to the configuration.
+ vaultManager.put(MetaStorageManager.APPLIED_REV, ByteUtils.longToBytes(configurationChangesCount + 1)).get();
+
+ try (var storage = new DistributedConfigurationStorage(wrapper.metaStorageManager(), vaultManager)) {
+ var changer = new TestConfigurationChanger(cgen, List.of(rootKey), Collections.emptyMap(),
+ storage, Collections.emptyList(), Collections.emptyList());
+
+ changer.start();
+
+ CompletableFuture<Long> longCompletableFuture = storage.lastRevision();
+
+ // Should return last configuration change, not last MetaStorage change.
+ Long lastConfigurationChangeRevision = longCompletableFuture.get();
+
+ assertEquals(configurationChangesCount, lastConfigurationChangeRevision);
+
+ changer.stop();
+ }
+ }
+
+ /**
+ * This class stores data for {@link MetaStorageManager}'s mock.
+ */
+ private static class MetaStorageMockWrapper {
+ private static final String DISTRIBUTED_PREFIX = "dst-cfg.";
+
+ /**
+ * This and previous field are copy-pasted intentionally, so in case if something changes,
+ * this test should fail and be reviewed and re-wrote.
Review Comment:
whoops
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950146465
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -185,38 +190,52 @@ public CompletableFuture<Serializable> readLatest(String key) {
/** {@inheritDoc} */
@Override
- public CompletableFuture<Data> readAll() throws StorageException {
+ public CompletableFuture<Data> readAllOnStart() throws StorageException {
return registerFuture(vaultMgr.get(MetaStorageManager.APPLIED_REV)
- .thenApplyAsync(appliedRevEntry -> {
- long appliedRevision = appliedRevEntry == null ? 0L : ByteUtils.bytesToLong(appliedRevEntry.value());
+ .thenComposeAsync(appliedRevEntry -> {
Review Comment:
Or we can take this even further and split `readAll` into two methods: one to resolve the required revision and another to read the data. Then it will look even better:
```
CompletableFuture<Data> future = vaultMgr.get(MetaStorageManager.APPLIED_REV)
.thenCombine(vaultMgr.get(CONFIGURATION_REVISIONS_KEY), this::resolveRevision)
.thenApplyAsync(this::readAll, threadPool);
return registerFuture(future);
```
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950144655
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -27,11 +27,11 @@
*/
public interface ConfigurationStorage extends AutoCloseable {
/**
- * Read all configuration values and current storage version.
+ * Reads all configuration values and current storage version on the start.
Review Comment:
Maybe we shall rename it to something like "performRecovery"?
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950172916
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorageTest.java:
##########
@@ -74,6 +93,145 @@ void stop() throws Exception {
vaultManager.stop();
}
+ /**
+ * Dummy configuration.
+ */
+ @ConfigurationRoot(rootName = "someKey", type = DISTRIBUTED)
+ public static class DistributedTestConfigurationSchema {
+ @Value(hasDefault = true)
+ public final int foobar = 0;
+ }
+
+ /**
+ * Tests that distributed configuration storage correctly picks up latest configuration MetaStorage revision
+ * during recovery process.
+ *
+ * @throws Exception If failed.
+ */
+ @Test
+ public void testMetaStorageRevisionDifferentFromConfigurationOnRestart() throws Exception {
+ RootKey<DistributedTestConfiguration, DistributedTestView> rootKey = DistributedTestConfiguration.KEY;
+
+ ConfigurationAsmGenerator cgen = new ConfigurationAsmGenerator();
+
+ MetaStorageMockWrapper wrapper = new MetaStorageMockWrapper();
+
+ int configurationChangesCount = 7;
Review Comment:
It looks cryptic indeed, can you add a comment?
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950189411
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -247,23 +266,6 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
operations.add(Operations.put(MASTER_KEY, ByteUtils.longToBytes(curChangeId)));
- // Condition for a valid MetaStorage data update. Several possibilities here:
Review Comment:
ok, let's remove it then
--
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] sashapolo commented on a diff in pull request #1023: IGNITE-17555 Fix AssertionError and NPE in configuration
Posted by GitBox <gi...@apache.org>.
sashapolo commented on code in PR #1023:
URL: https://github.com/apache/ignite-3/pull/1023#discussion_r950278841
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -572,20 +572,24 @@ private CompletableFuture<Void> updateFromListener(Data changedEntries) {
storageRoots = new StorageRoots(newSuperRoot, newChangeId);
- if (dataValuesPrefixMap.isEmpty()) {
- oldStorageRoots.changeFuture.complete(null);
-
- return CompletableFuture.completedFuture(null);
- } else {
- return notificator.notify(oldSuperRoot, newSuperRoot, newChangeId, notificationListenerCnt.incrementAndGet())
- .whenComplete((v, t) -> {
- if (t == null) {
+ // Save revisions for recovery.
+ return storage.writeConfigurationRevision(oldStorageRoots.version, storageRoots.version)
+ .thenCompose(unused -> {
+ if (dataValuesPrefixMap.isEmpty()) {
oldStorageRoots.changeFuture.complete(null);
+
+ return CompletableFuture.completedFuture(null);
} else {
- oldStorageRoots.changeFuture.completeExceptionally(t);
+ long notificationNumber = notificationListenerCnt.incrementAndGet();
+ return notificator.notify(oldSuperRoot, newSuperRoot, newChangeId, notificationNumber).whenComplete((v, t) -> {
Review Comment:
1. There should be a line break.
2. The same thing: I would suggest to move `whenComplete` to the next line
--
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