INGITE-14406 #3

alievmirza · 2021-04-08T12:30:01Z

No description provided.

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

...unner/src/main/java/org/apache/ignite/runner/internal/storage/LocalConfigurationStorage.java

modules/runner/src/main/java/org/apache/ignite/runner/internal/app/IgnitionImpl.java

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

vldpyatkov · 2021-04-08T16:06:49Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

+
+        version.incrementAndGet();
+
+        listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version.get(), 0)));


Here should be metastorage watches instead of direct call listeners.

Yep, according to current design it's not valid to trigger and process configuration listeners within write tread, only watch processing thread is expected.

vldpyatkov · 2021-04-08T16:07:36Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

+
+        listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version.get(), 0)));
+
+        return CompletableFuture.completedFuture(true);


The final future should aggregate from metastorage futures.

...unner/src/main/java/org/apache/ignite/runner/internal/storage/LocalConfigurationStorage.java

vldpyatkov · 2021-04-08T16:12:19Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/impl/VaultServiceImpl.java

+    /** Map to store values. */
+    private TreeMap<String, Value> storage = new TreeMap<>();
+
+    private final Object mux = new Object();


You can to make all methods synchronized with a similar success.

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

sk0x50 · 2021-04-08T16:16:11Z

modules/vault/README.md

@@ -0,0 +1,2 @@
+# Ignite vault module
+This module provides Vault API implementation.


To be honest, this description is very concise :)

sk0x50 · 2021-04-08T16:16:42Z

modules/vault/pom.xml

+        <dependency>
+            <groupId>org.rocksdb</groupId>
+            <artifactId>rocksdbjni</artifactId>
+            <version>6.6.4</version>


All versions must be placed at parent/pom.xml

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java

sk0x50 · 2021-04-08T16:25:00Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/common/Value.java

+/**
+ * Representation of vault entry.
+ */
+public class Value implements Serializable {


Do we really need revision for all values including local values (value of a local property/key)?

I planned to have revisions for all values, but local values would have revision equals to -1, for example

Vse ravno neponiatno, zachem?

sanpwc · 2021-04-08T17:04:28Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

+        HashMap<String, Serializable> data = new HashMap<>();
+
+        for (Entry entry : cur)
+            data.put(entry.key().toString().replace(DISTRIBUTED_PREFIX + ".", ""), (Serializable)SerializationUtils.fromBytes(entry.value()));


Seems that replace isn't safe enough cause entry.key().toString() could also have DISTRIBUTED_PREFIX + "." somewhere in the middle in addition to prefix.

sanpwc · 2021-04-08T17:11:43Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

+            data.put(entry.key().toString().replace(DISTRIBUTED_PREFIX + ".", ""), (Serializable)SerializationUtils.fromBytes(entry.value()));
+
+        // storage revision 0?
+        return new Data(data, version.get(), 0);


I'm not sure whether storageRevision is metastorage revision or key updateCounter. In any case it won't be 0 here. it'll be max revision or updateCounter that we saw within all entries that match rrange(new Key(DISTRIBUTED_PREFIX + "."), new Key(DISTRIBUTED_PREFIX + (char)('.' + 1)));

sanpwc · 2021-04-09T15:45:21Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

-    @Override public CompletableFuture<Boolean> write(Map<String, Serializable> newValues, long version) {
-        return null;
+    /** {@inheritDoc} */
+    @Override public synchronized CompletableFuture<Boolean> write(Map<String, Serializable> newValues, long sentVersion) {


Could you please clarify, why we need sentVersion?

sanpwc · 2021-04-09T15:52:06Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

+
+        version.incrementAndGet();
+
+        listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version.get(), 0)));


Yep, according to current design it's not valid to trigger and process configuration listeners within write tread, only watch processing thread is expected.

sanpwc · 2021-04-14T08:53:14Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

 import org.apache.ignite.metastorage.internal.MetaStorageManager;

 public class DistributedConfigurationStorage implements ConfigurationStorage {
+    // TODO Distributed prefix. Will be replaced when ENUM with configuration type will be presented.
+    // https://issues.apache.org/jira/browse/IGNITE-14476
+    private static String DISTRIBUTED_PREFIX = "distributed";


All keys within metastorage are distributed, I believe that you actually need "public" prefix here.

sanpwc · 2021-04-14T08:59:29Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

-    @Override public CompletableFuture<Boolean> write(Map<String, Serializable> newValues, long version) {
-        return null;
+        // storage revision 0?
+        return new Data(data, version, 0);


As far as I understand, instead of 0, you should use max revision that was retrieved from range().

sanpwc · 2021-04-14T09:19:35Z

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java

+
+        version++;
+
+        listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version, 0)));


It's not valid to use 0 here.

sanpwc · 2021-04-14T09:44:26Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/common/Value.java

+/**
+ * Representation of vault entry.
+ */
+public class Value implements Serializable {


Vse ravno neponiatno, zachem?

sanpwc · 2021-04-14T09:44:54Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/common/Value.java

+ * Representation of vault entry.
+ */
+public class Value implements Serializable {
+    private String key;


Why we need key in Value?

Seems that we don't need to store value with key in vault and we can store only byte array, but VaultService should return something like Entry in MetastorageService, so I'm going to introduce some kind of VaultEntry wich will be returned in org.apache.ignite.internal.vault.service.VaultService#get, for example

sanpwc · 2021-04-14T09:47:57Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/impl/VaultServiceImpl.java

+ */
+public class VaultServiceImpl implements VaultService {
+    /** Map to store values. */
+    private TreeMap<String, Value> storage = new TreeMap<>();


Same as mentioned above, why not to reuse Key abstraction from MetaStorage?

sanpwc · 2021-04-14T09:58:35Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java

+        return vaultService.appliedRevision(key);
+    }
+
+    public CompletableFuture<Void> put(String key, Value val) {


We'll need multi keys put with applied revision at least in order to commit processed watch notifications and multi keys put without applied revision for local keys.

sanpwc · 2021-04-14T10:00:33Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/common/Watch.java

+import java.util.Comparator;
+import org.jetbrains.annotations.Nullable;
+
+public class Watch {


Why not to reuse watch abstraction from DMS?

WatchListener depends on WatchEvent, which depends on Entry, Entry has revision and will have updateCounter in the future. Seems that it is not suitable for Vault as far as Vault don't need to have such fields.

sanpwc · 2021-04-14T14:29:20Z

modules/vault/src/main/java/org/apache/ignite/internal/vault/common/Watch.java

+    private static final Comparator<String> CMP = CharSequence::compare;
+
+    @Nullable
+    private String startKey;


Same as above, MetaStorage Key suites better here.

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

sk0x50 reviewed Apr 8, 2021

View reviewed changes

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java Show resolved Hide resolved

sk0x50 reviewed Apr 8, 2021

View reviewed changes

...unner/src/main/java/org/apache/ignite/runner/internal/storage/LocalConfigurationStorage.java Show resolved Hide resolved

vldpyatkov reviewed Apr 8, 2021

View reviewed changes

sk0x50 reviewed Apr 8, 2021

View reviewed changes

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java Show resolved Hide resolved

sk0x50 reviewed Apr 8, 2021

View reviewed changes

...src/main/java/org/apache/ignite/runner/internal/storage/DistributedConfigurationStorage.java Show resolved Hide resolved

sk0x50 reviewed Apr 8, 2021

View reviewed changes

modules/vault/src/main/java/org/apache/ignite/internal/vault/VaultManager.java Show resolved Hide resolved

sk0x50 reviewed Apr 8, 2021

View reviewed changes

sanpwc reviewed Apr 8, 2021

View reviewed changes

alievmirza force-pushed the ignite-14406 branch from db4794d to 7edc042 Compare April 9, 2021 10:17

vldpyatkov force-pushed the ignite-14410 branch from 6a63843 to a353102 Compare April 9, 2021 22:47

alievmirza force-pushed the ignite-14406 branch from 7edc042 to 93a41cb Compare April 12, 2021 12:35

sanpwc requested changes Apr 14, 2021

View reviewed changes

sanpwc reviewed Apr 14, 2021

View reviewed changes

alievmirza added 6 commits April 16, 2021 15:04

IGNITE-14406 vault implementation added

4ee578b

IGNITE-14406 watches added

4af0884

IGNITE-14406 watches added

7979efb

IGNITE-14406 watches added

7f0c28b

IGNITE-14406 watches added

82cc1a5

IGNITE-14406 wip

d6adcbc

alievmirza force-pushed the ignite-14406 branch 2 times, most recently from 81b147c to 84a63be Compare April 21, 2021 08:14

IGNITE-14406 added watch handling for DistributedConfigurationStorage

bdb77c0

alievmirza force-pushed the ignite-14406 branch from 84a63be to bdb77c0 Compare April 21, 2021 14:22

SammyVimes added a commit that referenced this pull request Apr 27, 2021

IGNITE-14088 Review fix #3

6b3d360

SammyVimes added a commit that referenced this pull request Apr 29, 2021

IGNITE-14088 Review fix #3

67e1a0a

SammyVimes added a commit that referenced this pull request May 6, 2021

IGNITE-14088 Review fix #3

5692e67

SammyVimes added a commit that referenced this pull request May 11, 2021

IGNITE-14088 Review fix #3

4119baf

tkalkirill added a commit that referenced this pull request Mar 30, 2022

IGNITE-16280 after review #3.1

6bcec14

tkalkirill added a commit that referenced this pull request Mar 30, 2022

IGNITE-16280 after review #3.2

9b50d1b

tkalkirill added a commit that referenced this pull request Mar 30, 2022

IGNITE-16280 after review #3.3

32b3139

tkalkirill added a commit that referenced this pull request Mar 30, 2022

IGNITE-16280 after review #3.4

a95e18b

tkalkirill added a commit that referenced this pull request Mar 31, 2022

IGNITE-16280 after review #3.5

b20890d

tkalkirill added a commit that referenced this pull request Mar 31, 2022

IGNITE-16280 after review #3.6

12d404d

tkalkirill added a commit that referenced this pull request May 26, 2022

IGNITE-16998 after review #3.0

f59adbf

tkalkirill added a commit that referenced this pull request May 26, 2022

IGNITE-16998 after review #3.0

b0d5170

tkalkirill added a commit that referenced this pull request Jul 19, 2022

IGNITE-17295 after review #3.0

ed5f719

korlov42 added a commit that referenced this pull request Aug 26, 2022

fix failed test #3

cb07de4

korlov42 added a commit that referenced this pull request Sep 22, 2022

fix bunch of tests #3

00885f5

korlov42 added a commit that referenced this pull request Sep 30, 2022

fix failed java tests #3

a116b35

tkalkirill added a commit that referenced this pull request Oct 13, 2022

IGNITE-17536 after review #3.1

9ac97d3

tkalkirill added a commit that referenced this pull request Oct 13, 2022

IGNITE-17536 after review #3.1

a70c5dd

tkalkirill added a commit that referenced this pull request Oct 13, 2022

IGNITE-17536 after review #3.3

6b2d045

korlov42 added a commit that referenced this pull request Oct 19, 2022

fix failed tests #3

c371553

korlov42 added a commit that referenced this pull request Oct 19, 2022

fix failed tests #3

0308be9

tkalkirill added a commit that referenced this pull request Nov 28, 2022

IGNITE-17132 after review #3.0

71816e2

tkalkirill added a commit that referenced this pull request Dec 12, 2022

IGNITE-18322 after review #3.0

29926e0

ibessonov added a commit that referenced this pull request Jan 18, 2023

IGNITE-18567 Rollback deleted validators #3

22b6c0a

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

tkalkirill added a commit that referenced this pull request Feb 24, 2023

IGNITE-18565 after review #3.0

08f6910

ibessonov added a commit that referenced this pull request Apr 25, 2023

IGNITE-19269 tests #3

f751dcf

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

ibessonov added a commit that referenced this pull request Jun 5, 2023

IGNITE-19593 Tests fixed, #3

8e6ea06

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

ibessonov added a commit that referenced this pull request Jun 27, 2023

IGNITE-19828 attempt #3

157a9b2

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

tkalkirill added a commit that referenced this pull request Sep 8, 2023

IGNITE-19499 after review #3.0

700a8d3

tkalkirill added a commit that referenced this pull request Sep 12, 2023

IGNITE-20396 fix tests #3.0

0b83946

tkalkirill added a commit that referenced this pull request Sep 22, 2023

IGNITE-20422 after review #3.0

a376864

tkalkirill added a commit that referenced this pull request Sep 22, 2023

IGNITE-20422 after review #3.1

c45b4fd

tkalkirill added a commit that referenced this pull request Jan 16, 2024

IGNITE-21155 after review #3.0

0159437

ibessonov added a commit that referenced this pull request Feb 23, 2024

IGNITE-21284 <checkstyle #3>

beb8bab

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INGITE-14406 #3

INGITE-14406 #3

alievmirza commented Apr 8, 2021

vldpyatkov Apr 8, 2021

sanpwc Apr 9, 2021

vldpyatkov Apr 8, 2021

vldpyatkov Apr 8, 2021

sk0x50 Apr 8, 2021

sk0x50 Apr 8, 2021

sk0x50 Apr 8, 2021

alievmirza Apr 9, 2021

sanpwc Apr 14, 2021

sanpwc Apr 8, 2021

sanpwc Apr 8, 2021

sanpwc Apr 9, 2021

sanpwc Apr 9, 2021

sanpwc Apr 14, 2021

sanpwc Apr 14, 2021

sanpwc Apr 14, 2021

sanpwc Apr 14, 2021

sanpwc Apr 14, 2021

alievmirza Apr 16, 2021 •

edited

Loading

sanpwc Apr 14, 2021

sanpwc Apr 14, 2021

sanpwc Apr 14, 2021

alievmirza Apr 14, 2021

sanpwc Apr 14, 2021


		version.incrementAndGet();

		listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version.get(), 0)));


		listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version.get(), 0)));

		return CompletableFuture.completedFuture(true);

		@@ -0,0 +1,2 @@
		# Ignite vault module
		This module provides Vault API implementation.


		version++;

		listeners.forEach(listener -> listener.onEntriesChanged(new Data(newValues, version, 0)));

INGITE-14406 #3

INGITE-14406 #3

Conversation

alievmirza commented Apr 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alievmirza Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alievmirza Apr 16, 2021 •

edited

Loading