Delivery improvements #1153

armiol · 2019-08-30T14:52:20Z

In this changeset, several improvements to the Delivery mechanism are made.

Consistent shard indexes

The default sharding strategy now relies on Murmur3 hashing algorithm, rather than on the native Java hashCode(). It allows determining the shard index consistently across different JVMs.

Delivery stages and monitoring

As long as the messages during the delivery are read page-by-page, the Delivery may now be configured with the page size. Additionally, an API user now can control whether the delivery should be continued after delivering another page. In environments, such as AppEngine Standard, there are time restrictions on the max duration of the request processing.

The usage of DeliveryMonitor allows to obtain more fine-grained control and stop the execution before the instance gets killed.

Batch dispatching

In case the consequent messages in the same shard are headed to the same target, they are now grouped in a batch and dispatched together. Also, instead of reading and writing the Entity per-message, the RepositoryCache is used to cache load and store calls.

It starts caching before the batch of InboxMessages is delivered and stops the caching afterward, propagating all the updates to the underlying storage.

Cleaning ShardedWorkRegistry

When an application node picks up a shard, a respective record is created in the ShardedWorkRegistry, telling that the node picked up some shard at a certain time. However, as the application nodes may get killed, some of the records may contain stale data and prevent other nodes from picking the shards.
The new ShardedWorkRegistry.releaseExpiredSessions(ShardIndex, Duration) call allows to manually clear the registry from the nodes which have picked up their sessions a long time ago (specified by the duration parameter).

Additionally, some polishing has been done in the API of Endpoints removing the parameter discrepancy and streamlining the execution.

… `Endpoint`.

…ess).

…he argument different from the envelope wrapped.

…a `RepositoryCache` (in progress).

…on` caches.

Extract nested classes from `Inbox` and `InboxPart`.

… not been active for too long.

Add more tests for delivery.

codecov · 2019-08-30T15:12:41Z

Codecov Report

Merging #1153 into master will decrease coverage by 0.04%.
The diff coverage is 89.33%.

@@             Coverage Diff              @@
##             master    #1153      +/-   ##
============================================
- Coverage     91.73%   91.69%   -0.05%     
- Complexity     4112     4190      +78     
============================================
  Files           536      542       +6     
  Lines         12989    13210     +221     
  Branches        747      767      +20     
============================================
+ Hits          11916    12113     +197     
- Misses          838      850      +12     
- Partials        235      247      +12

dmdashenkov

@armiol, please see my comments.

dmdashenkov · 2019-08-30T15:34:58Z

server/src/main/java/io/spine/server/aggregate/AggregateRepository.java

+    }
+
+    @VisibleForTesting
+    protected A doLoadOrCreate(I id) {


Can we make these methods package-private? For a user, it is strange to see them when subclassing AggregateRepository.

dmdashenkov · 2019-08-30T15:36:46Z

server/src/main/java/io/spine/server/delivery/Delivery.java

@@ -59,17 +58,19 @@
 *
 * <b>Configuration</b>
 *
- * <p>By default, a shard is assigned according to the identifier of the target entity. The messages
+ * <p>By default, a shard is assigned according to the identifier of the target entity. The
+ * messages


Please fix the alignment here and further down the doc.

dmdashenkov · 2019-08-30T15:39:02Z

server/src/main/java/io/spine/server/delivery/Delivery.java

+ * {@linkplain DeliveryBuilder#setPageSize(int) configured}.
+ *
+ * <p>After each {@code DeliveryStage} it is possible to stop the delivery by
+ * {@link DeliveryBuilder#setMonitor(DeliveryMonitor) supplying} the custom delivery monitor.


Suggested change

* {@link DeliveryBuilder#setMonitor(DeliveryMonitor) supplying} the custom delivery monitor.

* {@link DeliveryBuilder#setMonitor(DeliveryMonitor) supplying} a custom delivery monitor.

dmdashenkov · 2019-08-30T15:39:51Z

server/src/main/java/io/spine/server/delivery/Delivery.java

        try {
+


Please kill this line.

dmdashenkov · 2019-08-30T15:43:03Z

server/src/main/java/io/spine/server/delivery/Delivery.java

            int deliveredMsgCount;
            do {
                deliveredMsgCount = doDeliver(session);
            } while (deliveredMsgCount > 0);
+        } catch (DeliveryStoppedByMonitorException ignored) {
+            // do nothing.


Let's group the deliveredMsgCount and the instructions of whether to continue delivery or not into an object. Something of a sort:

StageResult result; do { result = doDeliver(session); } while (result.shouldDeliverMore());

dmdashenkov · 2019-08-30T15:56:05Z

server/src/main/java/io/spine/server/delivery/UniformAcrossAllShards.java

@@ -32,12 +36,26 @@
 *
 * <p>Uses a hash code of the entity identifier and the remainder of its division by the total
 * number of shards to determine the index of a shard, at which the modification is allowed.
+ *
+ * <p>To reach the consistency of the calculated values across different nodes,
+ * the identifier is transformed into a {@code String}, and this value is then hashed


the identifier is transformed into a String

This is not true for Message identifiers. I'd just say that it is transformed into bytes.

dmdashenkov · 2019-08-30T15:58:48Z

server/src/main/java/io/spine/server/delivery/UniformAcrossAllShards.java

+     * The hash function to use for the shard index calculation.
+     */
+    @SuppressWarnings("UnstableApiUsage")   // See the class-level docs.
+    private static final HashFunction HASHER = Hashing.murmur3_32(0);


The seed 0 may just be avoided (and the no-argument method may be called instead).

dmdashenkov · 2019-08-30T16:04:36Z

server/src/main/java/io/spine/server/entity/RepositoryCache.java

+     * @param <E>
+     *         the type of entity
+     */
+    public interface Store<I, E extends Entity<I, ?>> extends Consumer<E> {


Why do we need I?

dmdashenkov · 2019-08-30T16:05:19Z

server/src/main/java/io/spine/server/entity/RepositoryCache.java

+
+        if (!cache.containsKey(idInTenant)) {
+            E entity = loadFn.apply(idInTenant.value());
+            cache.put(idInTenant, entity);


Can the cache grow too quick and fill the memory?

Not in our use cases. The maximum number of cached entities is the total number of shards — and that's in case there is just one node serving the whole app. It's because each shard delivery is a single-thread process, and the only entity is cached at a time by the delivery.

End-users will hardly ever have 100k of shards and just a single instance processing them all.

I don't want to use Guava's cache for such a simple use case. And it's much slower than the concurrent HashMap.

I have added the note on usage though.

dmdashenkov · 2019-08-30T16:10:01Z

server/src/test/java/io/spine/server/delivery/InboxIdsTest.java

+        super.configure(tester);
+        tester.setDefault(InboxId.class, InboxId.getDefaultInstance())
+              .setDefault(TypeUrl.class, TypeUrl.of(Calc.class));
+        tester.testStaticMethods(getUtilityClass(), NullPointerTester.Visibility.PACKAGE);


The configure method is not meant to execute tests.

Update the license report one more time.

dmdashenkov

@armiol, please consider making Store and Load functional interfaces. Other than that — LGTM.

Update the license report.

armiol added 22 commits August 23, 2019 19:46

Use Guava's Hashing to calculate the shard index consistently.

9d80eec

Remove a redundant suppression of the warning.

a8798e5

Extract the Delivery.Builder into a top-level class.

195a2f7

Introduce a DeliveryMonitor.

094d885

Remove a redundant line.

2967153

Fix the Javadoc method reference.

a63a3cf

Do not allow to dispatch any other message than what's wrapped by the…

b78ef8b

… `Endpoint`.

Deliver messages to the same target/label/tenant in a batch (in progr…

8df19e0

…ess).

Do not allow to call EntityMessageEndpoint.onEmptyResult(..) with t…

a244403

…he argument different from the envelope wrapped.

Implement a batch dispatching of message from Inbox by introducing …

63384d9

…a `RepositoryCache` (in progress).

Call proper load/store methods for ProcessManager and `Projecti…

67c08c6

…on` caches.

Resolve test issues related to over-mocking.

f72b9c9

Start testing the batched delivery (in progress).

ad46c9c

Extract nested classes from `Inbox` and `InboxPart`.

Test the delivery in batches.

d16d8e0

Fix the display name.

056b24c

Clear the stat counters.

2be21c2

Get rid of System.out call.

18c142f

Calculate the shard index without using Stringifiers.

917b87d

Allow to release the node values from the shard indexes if a node has…

f4f6ef9

… not been active for too long.

Allow to customize the page size for a DeliveryStage.

e38ae60

Add the tests for InboxStorage and InboxIds.

7a24405

Allow to monitor the delivery stages after each page is delivered.

18d72ad

Add more tests for delivery.

armiol self-assigned this Aug 30, 2019

armiol added 2 commits August 30, 2019 17:59

Rename and document the class.

7572ad8

Document the class.

4503891

armiol added 4 commits August 30, 2019 18:13

Kill the redundant lines.

71a819c

Merge remote-tracking branch 'origin/master' into delivery-improvements

cf30b35

Use the charset explicitly.

dc54ab9

Update the Spine version.

beb8315

armiol marked this pull request as ready for review August 30, 2019 15:23

armiol requested a review from dmdashenkov August 30, 2019 15:23

armiol added 2 commits August 30, 2019 19:02

Improve the code layout.

2602fd4

Pass the proper value to the monitor.

be073df

dmdashenkov suggested changes Aug 30, 2019

View reviewed changes

armiol added 11 commits August 30, 2019 19:31

Avoid the flow-control exception.

e917e82

Update the license report and pom.xml

e08ee1e

Fix the alignment.

6fd3083

Fix the tag and format properly.

3fa8534

Get rid of a space character.

2e07e49

Address the discrepancy between the docs and the implementation.

fe4c6c1

Use the default factory method instead of seeding the default value.

270e6f2

Remove the unused generic parameter.

3de308e

Add a note on cache usage.

637da8f

Fix the test flow.

360aba1

Turn the DeliveryStage into Protobuf message.

3153ff2

Update the license report one more time.

armiol requested a review from dmdashenkov August 30, 2019 17:39

dmdashenkov approved these changes Aug 30, 2019

View reviewed changes

armiol added 2 commits August 31, 2019 15:27

Mark RepositoryCache's nested interfaces as functional.

feb7f61

Update the license report.

Notify the batch delivery listener in any case.

0ab5620

armiol merged commit 1ca7d12 into master Aug 31, 2019

armiol deleted the delivery-improvements branch August 31, 2019 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delivery improvements #1153

Delivery improvements #1153

armiol commented Aug 30, 2019 •

edited

Loading

codecov bot commented Aug 30, 2019 •

edited

Loading

dmdashenkov left a comment

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov Aug 30, 2019

armiol Aug 30, 2019

dmdashenkov Aug 30, 2019

dmdashenkov left a comment

	* {@link DeliveryBuilder#setMonitor(DeliveryMonitor) supplying} the custom delivery monitor.
	* {@link DeliveryBuilder#setMonitor(DeliveryMonitor) supplying} a custom delivery monitor.

Delivery improvements #1153

Delivery improvements #1153

Conversation

armiol commented Aug 30, 2019 • edited Loading

codecov bot commented Aug 30, 2019 • edited Loading

Codecov Report

dmdashenkov left a 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

dmdashenkov left a comment

Choose a reason for hiding this comment

armiol commented Aug 30, 2019 •

edited

Loading

codecov bot commented Aug 30, 2019 •

edited

Loading