Use an explicit per-View modification counter [ECR-2804] #658

dmitry-timofeev · 2019-01-15T17:10:34Z

Overview

Use an explicit modification counter in each View to get rid of
static state in ViewModificationCounter singleton and simplify
testing. As previously, a ModificationCounter is associated with each
View (either a Fork or Snapshot), but this relationship is now more
transparent.

See: https://jira.bf.local/browse/ECR-2804

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

Use an explicit modification counter in each View to get rid of static state in ViewModificationCounter singleton and simplify testing. As previously, a ModificationCounter is associated with each View (either a Fork or Snapshot), but this relationship is now more transparent.

dmitry-timofeev · 2019-01-15T17:11:50Z

...java-binding-core/src/main/java/com/exonum/binding/storage/database/ModificationCounter.java

+  /**
+   * Notifies this counter that the source object is modified, updating its current value.
+   *
+   * @throws IllegalStateException if this counter corresponds to a read-only (immutable) object,


Not sure if a counter shall concern itself with immutable things (on top of that, the AbstractIndexProxy does this check itself)

I think it's better to be safe and throw an exception in case of an immutable counter (as it is now)

dmitry-timofeev · 2019-01-15T17:13:12Z

exonum-java-binding-core/src/test/java/com/exonum/binding/storage/database/ForkTest.java

 })
 @Disabled
-// TODO Won't run on Java 10 till Powermock is updated [ECR-1614].
+// TODO Won't run on Junit 5 till Powermock is updated [ECR-1614].


to be more precise, till Powermock gains support of JUnit 5 (we use the most recent version)

dmitry-timofeev · 2019-01-15T17:15:36Z

...va-binding-core/src/test/java/com/exonum/binding/storage/indices/AbstractIndexProxyTest.java

    assertThat(thrown.getMessage(), matchesPattern(pattern));
+
+    assertFalse(counter.isModifiedSince(initialValue));
  }

  @Test
  void notifyModifiedAcceptsFork() {


This test verifies that the index does use a counter from the Fork. Not sure if we shall add an integration test of Collection + Iterator + ModificationCounter, verifying that modification of another collection created with the same Fork invalidates the iterator of the first collection.

MakarovS

👍

MakarovS · 2019-01-16T22:07:12Z

...java-binding-core/src/main/java/com/exonum/binding/storage/database/ModificationCounter.java

+package com.exonum.binding.storage.database;
+
+/**
+ * A counter of events-modifications of some objects (e.g., a collection, or a database view).


A counter of modification events?

MakarovS · 2019-01-16T22:08:39Z

...java-binding-core/src/main/java/com/exonum/binding/storage/database/ModificationCounter.java

+  /**
+   * Notifies this counter that the source object is modified, updating its current value.
+   *
+   * @throws IllegalStateException if this counter corresponds to a read-only (immutable) object,


I think it's better to be safe and throw an exception in case of an immutable counter (as it is now)

MakarovS · 2019-01-16T22:22:05Z

...java-binding-core/src/main/java/com/exonum/binding/storage/indices/ConfigurableRustIter.java

@@ -32,8 +32,10 @@
 final class ConfigurableRustIter<E> extends AbstractNativeProxy implements RustIter<E> {

  private final LongFunction<E> nextFunction;
+  // todo: This view is kept only for diagnostic purposes (to put in error message) —


Views don't have toString overridden, so would it be helpful to log things like com.exonum.binding.storage.database.Fork@131ef10? Or maybe it would make sense to return View#getViewNativeHandle for diagnostic purposes? If no to both questions, then I'd remove this field.

MakarovS · 2019-01-16T22:29:38Z

...java-binding-core/src/main/java/com/exonum/binding/storage/database/ModificationCounter.java

+   */
+  boolean isModifiedSince(int lastValue);
+
+  // todo: We _might_ consider adding a special type for this timestamp, but at the moment it feels like an overkill


Maybe create a task instead of todo? Do you think it's realistic that a counter might overflow int range? If yes, we could add throwing an exception in notifyModified for such a case

The reasoning was to hide the knowledge of how that timestamp is represented to be able to change later, but in an internal interface it seems to be unnecessary.

I've documented, however, that counters must detect up to 4B modifications, that shall be enough :-)

MakarovS · 2019-01-16T22:37:57Z

...g-core/src/main/java/com/exonum/binding/storage/database/IncrementalModificationCounter.java

+
+final class IncrementalModificationCounter implements ModificationCounter {
+
+  private static final int INITIAL_VALUE = 0;


Seems excessive to me, as we only use it once in the next line. Or maybe make it public? That way it'd be clear for users what the initial value is (because right now in tests to get initialValue we call getCurrentValue which is error-prone. We could use a public field in tests as well). But I do not insist, it's good as it is as well :)

I do not think that the clients shall care about the initial value, what they shall is current value (= the value at the moment when an (spl)iterator is bound to source). initial value is an implementation detail of this counter.

The constnant can probably be inlined, and the field renamed to counter

exonum-java-binding-core/src/main/java/com/exonum/binding/storage/database/Fork.java

…age/database/Fork.java Co-Authored-By: dmitry-timofeev <[email protected]>

…num-java-binding-1 into refactor-mod-counter

coveralls · 2019-01-17T17:14:29Z

Coverage decreased (-0.02%) to 84.144% when pulling 9c96b2e on dmitry-timofeev:refactor-mod-counter into 6b02435 on exonum:master.

dmitry-timofeev added 2 commits January 15, 2019 16:42

Update Powermock

82dead4

dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jan 15, 2019

dmitry-timofeev requested review from bullet-tooth and MakarovS January 15, 2019 17:10

dmitry-timofeev commented Jan 15, 2019

View reviewed changes

dmitry-timofeev mentioned this pull request Jan 16, 2019

Add ListProxy#stream [ECR-2796]: #661

Merged

7 tasks

MakarovS approved these changes Jan 16, 2019

View reviewed changes

bullet-tooth reviewed Jan 17, 2019

View reviewed changes

exonum-java-binding-core/src/main/java/com/exonum/binding/storage/database/Fork.java Outdated Show resolved Hide resolved

dmitry-timofeev and others added 6 commits January 17, 2019 17:44

Remove dependency of RustIter on View

bb7a4f1

Clarify documentation

4bcc1b6

Remove initial_value

f227820

Update exonum-java-binding-core/src/main/java/com/exonum/binding/stor…

70f0f9f

…age/database/Fork.java Co-Authored-By: dmitry-timofeev <[email protected]>

Merge branch 'refactor-mod-counter' of github.com:dmitry-timofeev/exo…

80c7ba9

…num-java-binding-1 into refactor-mod-counter

Merge remote-tracking branch 'main/master' into refactor-mod-counter

2cad55b

dmitry-timofeev removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jan 17, 2019

dmitry-timofeev added 2 commits January 17, 2019 17:59

Update changelog

16b6f6d

Fix checkstyle

9c96b2e

MakarovS approved these changes Jan 17, 2019

View reviewed changes

Merge branch 'master' into refactor-mod-counter

206a972

dmitry-timofeev merged commit a20608c into exonum:master Jan 18, 2019

dmitry-timofeev deleted the refactor-mod-counter branch January 18, 2019 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use an explicit per-View modification counter [ECR-2804] #658

Use an explicit per-View modification counter [ECR-2804] #658

dmitry-timofeev commented Jan 15, 2019 •

edited

Loading

dmitry-timofeev Jan 15, 2019

MakarovS Jan 16, 2019 •

edited

Loading

dmitry-timofeev Jan 15, 2019

dmitry-timofeev Jan 15, 2019

MakarovS left a comment

MakarovS Jan 16, 2019

MakarovS Jan 16, 2019 •

edited

Loading

MakarovS Jan 16, 2019

MakarovS Jan 16, 2019

dmitry-timofeev Jan 17, 2019

MakarovS Jan 16, 2019

dmitry-timofeev Jan 17, 2019

coveralls commented Jan 17, 2019


		final class IncrementalModificationCounter implements ModificationCounter {

		private static final int INITIAL_VALUE = 0;

Use an explicit per-View modification counter [ECR-2804] #658

Use an explicit per-View modification counter [ECR-2804] #658

Conversation

dmitry-timofeev commented Jan 15, 2019 • edited Loading

Overview

Definition of Done

Choose a reason for hiding this comment

MakarovS Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakarovS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakarovS Jan 16, 2019 • 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

coveralls commented Jan 17, 2019

dmitry-timofeev commented Jan 15, 2019 •

edited

Loading

MakarovS Jan 16, 2019 •

edited

Loading

MakarovS Jan 16, 2019 •

edited

Loading