Add initial ServiceRuntime for Dynamic Services [ECR-3404] #1065

dmitry-timofeev · 2019-08-13T13:08:18Z

Overview

Adds initial implementation of ServiceRuntime operations for dynamic services.

The code in dependent on core modules has not been migrated as the ServiceRuntime interface is not finalized yet, neither are the user-facing interfaces (like Service, Transaction, etc.).

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

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

dmitry-timofeev · 2019-08-13T15:30:33Z

How to review:

Java: everything, but start with:
- ServiceRuntime
- ServiceRuntimeIntegrationTest
- ServiceFactory & its implementation and test
- Service
- ServiceWrapper
- everything else
Rust:
- (for native interface) ServiceRuntimeAdapter and ServiceRuntime
- jni_cache

dmitry-timofeev · 2019-08-13T15:31:55Z

+1,612 −1,605 — nothing was added, actually 🙃

bullet-tooth · 2019-08-14T12:26:09Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceInstanceSpec.java

+  public abstract String getName();
+
+  /**
+   * Returns the numeric id of the service instance. Exonum assigns it to the service


Exonum assigns it to the service on instantiation.

As I remember, a user must provide id for its service which is unique within the network. Does it change?

Yes, it has been changed in dynamic services. As administrators can create several instances from the same artifact, the service id cannot be hardcoded in the service definition, but must be assigned on instantiation, so that the transaction messages can be routed to the correct instance. The id assignment is done by the core; the name is provided by the administrators (and is considered the primary identifier of the service instance).

bullet-tooth · 2019-08-14T12:28:41Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntime.java

-    this.frameworkInjector = checkNotNull(frameworkInjector);
    this.serviceLoader = checkNotNull(serviceLoader);
+    this.servicesFactory = checkNotNull(servicesFactory);
+    services = new TreeMap<>();


I would move instantiations to the field ^

What is the reasoning behind your suggestion? I can think of bringing the definition closer to the field declaration 🤔

MakarovS

👍

MakarovS · 2019-08-15T02:47:51Z

exonum-java-binding/core/rust/src/utils/jni_cache.rs

 static mut CLASS_GET_NAME: Option<JMethodID> = None;
 static mut THROWABLE_GET_MESSAGE: Option<JMethodID> = None;

+// todo: Remove transaction and service adapter items when native JavaServiceRuntime is implemented


Link to the task?

There is one 'Implement JavaServiceRuntime' in the native, but it isn't in Jira till some questions on it are clarified: https://wiki.bf.local/display/EJB/Java+Dynamic+Services+P1+Design#JavaDynamicServicesP1Design-%D0%97%D0%B0%D0%B4%D0%B0%D1%87%D0%B8

MakarovS · 2019-08-15T02:51:09Z

...um-java-binding/core/src/main/java/com/exonum/binding/core/runtime/GuiceServicesFactory.java

@@ -0,0 +1,43 @@
+package com.exonum.binding.core.runtime;


MakarovS · 2019-08-15T02:56:40Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/Pf4jServiceLoader.java

  }

-  private static ServiceId extractServiceId(String pluginId) throws ServiceLoadingException {
+  private static ServiceArtifactId extractServiceId(String pluginId)


extractServiceArtifactId?

I suggest keeping it as is for brevity — it must be clear from the context.

MakarovS · 2019-08-15T03:00:29Z

...-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceFrameworkModule.java

@@ -0,0 +1,23 @@
+package com.exonum.binding.core.runtime;


MakarovS · 2019-08-15T03:00:57Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceInstanceSpec.java

@@ -0,0 +1,35 @@
+package com.exonum.binding.core.runtime;


MakarovS · 2019-08-15T04:49:17Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java

@@ -0,0 +1,77 @@
+package com.exonum.binding.core.runtime;


MakarovS · 2019-08-15T04:50:33Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServicesFactory.java

@@ -0,0 +1,25 @@
+package com.exonum.binding.core.runtime;


MakarovS · 2019-08-15T04:53:24Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/service/Service.java

  /**
-   * Converts an Exonum raw transaction to an executable transaction of <em>this</em> service.
+   * Returns a list of hashes representing the state of this service, as of the given snapshot
+   * of the blockchain state. Usually, it includes the root hashes of all Merkelized collections


Root hashes or object/index hashes?

Yes, thanks!

MakarovS · 2019-08-15T04:56:36Z

...um-java-binding/core/src/main/java/com/exonum/binding/core/service/TransactionConverter.java

+   * @param arguments the {@linkplain TransactionMessage#getPayload() serialized transaction
+   *     arguments}
+   * @return an executable transaction of the service
+   * @throws IllegalArgumentException if the raw transaction is malformed or not known


Shouldn't we rephrase this exception description?

MakarovS · 2019-08-15T05:05:25Z

exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceWrapperTest.java

@@ -0,0 +1,93 @@
+package com.exonum.binding.core.runtime;


vitvakatu · 2019-08-15T07:32:37Z

Looks good to me, but where is beforeCommit method? Can't find any task for it and it is mentioned in documentation of ServiceRuntime.

dmitry-timofeev · 2019-08-15T10:16:37Z

beforeCommit

It's under 8 in https://wiki.bf.local/display/EJB/Java+Dynamic+Services+P1+Design#JavaDynamicServicesP1Design-%D0%97%D0%B0%D0%B4%D0%B0%D1%87%D0%B8

(or keep as is?)

- Perform the expected/actual id verification

* Remove: - getId — this one might be reconsidered - getName * Update configure to take configuration parameters and return void * Make getStateHashes implementation required (as no state hashes usually means "you don't need the blockchain or do it wrong"). Obviously, it neither compiles nor works.

#createService does not connect it to the Server yet — that is delayed till some questions are resolved.

It has a potential issue with memory usage which we shall consider together with the updates in Fork required for isolation of database modifications from different services in prospective beforeCommit

Also, pass them to native in Protobuf. No 3D arrays any longer.

ViewFactory is moved to the runtime package, as its main usages are there. Code mounting the API is still to be taken from UserServiceAdapter when the appropriate operation is added to the ServiceRuntime. Usages of UserServiceAdapter in the Testkit are left as is.

Move ServiceRuntime to integration tests as it uses the native library. Don't initialize the method ids of no longer existing classes when the native library is loaded. Setup some invocations.

Add Jira references where appropriate

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

dmitry-timofeev force-pushed the service-runtime-for-ds-ECR-3404 branch 2 times, most recently from 7c135d4 to 0ba481a Compare August 13, 2019 13:44

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

dmitry-timofeev requested review from bullet-tooth, MakarovS, skletsun and vitvakatu and removed request for bullet-tooth August 13, 2019 15:27

bullet-tooth reviewed Aug 14, 2019

View reviewed changes

MakarovS reviewed Aug 15, 2019

View reviewed changes

vitvakatu approved these changes Aug 15, 2019

View reviewed changes

dmitry-timofeev added 13 commits August 20, 2019 14:50

Rename ServiceId -> ServiceArtifactId

647176a

(or keep as is?)

Update the ServiceRuntime#loadArtifact

38f3d4f

- Perform the expected/actual id verification

Add ServiceInstanceSpec

2e492c1

Clarify the service instance Javadoc

9cebb21

Update ServiceRuntime#createService and add #configureService.

d3df4b9

#createService does not connect it to the Server yet — that is delayed till some questions are resolved.

Add stopService

54c3fd6

Extract a factory of Services from the ServiceRuntime

7bb2c0f

Implement transaction execution

ecc4aa0

Move the tests needing a started service into the @nested test

c2491d1

Add #getStateHashes

7bbc2d9

Fix checkstyle

3191e11

Add afterCommit:

ff7c25b

It has a potential issue with memory usage which we shall consider together with the updates in Fork required for isolation of database modifications from different services in prospective beforeCommit

dmitry-timofeev added 17 commits August 20, 2019 14:51

Clarify comment on afterCommit memory management

f125d59

Add a ServiceRuntimeAdapter with a native-code tailored interface.

afe6f6f

Update #stateHashes to return all the needed information

881148a

Also, pass them to native in Protobuf. No 3D arrays any longer.

Clarify the choice of representation

6b558de

Clarify RawTransaction fate

5af96bc

Fix compilation

2541783

Fix the core tests:

49ba583

Move ServiceRuntime to integration tests as it uses the native library. Don't initialize the method ids of no longer existing classes when the native library is loaded. Setup some invocations.

Skip the tests past core module

e72f8b1

Clarify/resolve todos:

b534355

Add Jira references where appropriate

Fix spotbugs issues

48a160b

Fix coverage reporting

6dfd8b1

Clarify the javadoc

8e91e94

Bring field definitions closer to their declarations

6cbb34d

Add missing copyrights

47be52c

Not root hashes

68e12d7

Clarify when the exception is appropriate

4f54926

dmitry-timofeev force-pushed the service-runtime-for-ds-ECR-3404 branch from aadf8a7 to 4f54926 Compare August 20, 2019 11:53

dmitry-timofeev changed the base branch from merkledb to dynamic-services August 20, 2019 11:53

dmitry-timofeev changed the base branch from dynamic-services to master August 20, 2019 11:58

dmitry-timofeev changed the base branch from master to dynamic-services August 20, 2019 11:58

dmitry-timofeev merged commit d95151d into exonum:dynamic-services Aug 20, 2019

dmitry-timofeev deleted the service-runtime-for-ds-ECR-3404 branch August 20, 2019 12:01

Add initial ServiceRuntime for Dynamic Services [ECR-3404] #1065

Add initial ServiceRuntime for Dynamic Services [ECR-3404] #1065

Uh oh!

Conversation

dmitry-timofeev commented Aug 13, 2019

Overview

Definition of Done

Uh oh!

dmitry-timofeev commented Aug 13, 2019

Uh oh!

dmitry-timofeev commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MakarovS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitvakatu commented Aug 15, 2019

Uh oh!

dmitry-timofeev commented Aug 15, 2019

Uh oh!

Uh oh!