[ECR-4133] Add service resume #1372

bullet-tooth · 2020-01-21T07:11:54Z

Overview

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

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

bullet-tooth · 2020-01-21T07:20:04Z

...m-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapter.java

+   *     message
+   * @see ServiceRuntime#initializeResumingService(ServiceInstanceSpec, byte[])
+   */
+  void initializeResumingService(byte[] instanceSpec, byte[] configuration) {


@vitvakatu method signature for ECR-4134

coveralls · 2020-01-21T07:31:58Z

Coverage increased (+0.02%) to 85.064% when pulling 367b32e on ecr-4133 into 6aabf29 on master.

dmitry-timofeev

I don't understand the Service#resume operation at all — it is inconsistent with node restarts; it is not possible to do anything from it — see the inline discussion.

dmitry-timofeev · 2020-01-21T11:13:00Z

exonum-java-binding/CHANGELOG.md

@@ -36,7 +36,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - `supervisor-mode` CLI parameter added for `generate-template` command. It
  allows to configure the mode of the Supervisor service. Possible values are
  "simple" and "decentralized". (#1361)
- Service instances can be stopped now. (#1358)
+- Service instances can be stopped and resumed now. (#1358, #1372)


An instance artifact may be upgraded before resuming, but with no data changes.

(may also insert the uses cases from the epic/ECR-3773).

dmitry-timofeev · 2020-01-21T11:13:31Z

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

@@ -53,6 +53,24 @@ default void initialize(Fork fork, Configuration configuration) {
    // No configuration
  }

+  /**
+   * Performs resuming of previously stopped service instance. This method is called <em>once</em>


* Resumes the previously stopped service instance. This method is called when * a stopped service instance is restarted.

dmitry-timofeev · 2020-01-21T11:14:53Z

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

+   *     the registry of call errors} if the resuming fails
+   * @see Configurable
+   */
+  default void resume(Configuration configuration) {


@slowli, @bullet-tooth In which cases the service needs to be notified of that, if it can't do anything with this configuration, just look at them? I remember that the service instance objects are not supposed to have much state, therefore,

"As an example, arguments can be used to update the service configuration."

does not seem to make much sense — that configuration must be normally kept in the persistent storage. On top of that, we do not provide a similar operation for node restarts, when a fresh service instance object is also created, hence "update in the object instance" argument cannot be valid.

dmitry-timofeev · 2020-01-21T11:15:22Z

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

+   *
+   * @param configuration the service configuration parameters
+   * @throws ExecutionException if the configuration parameters are not valid (e.g.,
+   *     malformed, or do not meet the preconditions). Exonum will save the error into


It will not save them.

dmitry-timofeev · 2020-01-21T11:15:44Z

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

+   *     malformed, or do not meet the preconditions). Exonum will save the error into
+   *     {@linkplain com.exonum.binding.core.blockchain.Blockchain#getCallErrors(long)
+   *     the registry of call errors} if the resuming fails
+   * @see Configurable


Configurable is not related to this, but initialize (if we decide to keep the method at all)

dmitry-timofeev · 2020-01-21T11:17:08Z

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

+      checkStoppedService(instanceSpec.getId());
+      ServiceWrapper service = createServiceInstance(instanceSpec);
+      service.resume(new ServiceConfiguration(configuration));
+    }


Currently this class is responsible to log errors, though there is ECR-3813 to shift that.

dmitry-timofeev · 2020-01-21T11:17:24Z

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

+  /** Checks that the service with the given id is not active in this runtime. */
+  private void checkStoppedService(Integer serviceId) {
+    checkArgument(!servicesById.containsKey(serviceId),
+        "Service with id=%s should be stopped, but actually active", serviceId);


Add the offending instance?

dmitry-timofeev · 2020-01-21T11:17:44Z

...inding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java

@@ -96,8 +96,6 @@ void startsServerOnInitialization() {

  @Test
  void deployCorrectArtifact() throws Exception {
-    serviceRuntime.initialize(mock(Node.class));


(Here and elsewhere) Why is this removed? No operation shall occur before SR is initialized.

ok, I will add a null check to the decorator then

Not sure I understand — the SR expects that it is first initialized with a non-null Node before any other operations are performed.

dmitry-timofeev · 2020-01-21T11:18:30Z

...inding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java

+    // and the service was configured
+    Configuration expectedConfig = new ServiceConfiguration(configuration);
+    verify(serviceWrapper).resume(expectedConfig);
+  }


// but not registered in the runtime yet:
assertThat(serviceRuntime.findService(TEST_NAME)).isEmpty();

dmitry-timofeev

configuration does not seem appropriate in the #resume method.

dmitry-timofeev · 2020-01-22T11:07:15Z

exonum-java-binding/CHANGELOG.md

@@ -36,7 +36,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - `supervisor-mode` CLI parameter added for `generate-template` command. It
  allows to configure the mode of the Supervisor service. Possible values are
  "simple" and "decentralized". (#1361)
- Service instances can be stopped now. (#1358)
+- Support of service instances lifecycle: they can be activated, stopped and resumed now.
+  Also, service instance artifacts can be upgraded before resuming (but with no data changes)


No data changes, according to the recent discussions, is no longer correct.

I'd add to the list below 'synchronous data migration'.

dmitry-timofeev · 2020-01-22T11:08:03Z

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

+   * parameters, deprecate old entries etc.
+   *
+   * <p>Also, note that performing any bulk operations or data migration
+   * <em>is not recommended</em> here.


I'd add short motivation, as Ostrovski clarified in the discussion.

dmitry-timofeev · 2020-01-22T11:08:33Z

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

+   * <!--TODO: Add a link to the migration procedure -->
+   *
+   * @param fork a database fork to apply changes to. Not valid after this method returns
+   * @param configuration the service configuration parameters


As we've discussed, those are hardly configuration parameters, just generic 'arguments'.

there are 2 questions here:

should we use Configuration type or byte[]? Configuration looks more convenient

If we use Configuration class for the resume operation then it's javadoc and, probably, name (ServiceArgumests, ServiceParameters?) should be updated.

Also, talking about Configuration, what is the reason for having interface + implementation(ServiceConfiguration), not just VO class?

byte[] since is it is not configuration. We shall not use Configuration for this operation — it is for configuration parameters.

It could be made one, it is made in this way probably to give more flexibility in terms of implementation (e.g., if we went with also properties).

dmitry-timofeev · 2020-01-22T11:08:42Z

...inding/core/src/test/java/com/exonum/binding/core/runtime/ServiceRuntimeIntegrationTest.java

+    verify(servicesFactory).createService(eq(serviceDefinition), eq(instanceSpec),
+        any(MultiplexingNodeDecorator.class));
+
+    // and the service was configured


dmitry-timofeev · 2020-01-23T08:38:37Z

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

+   * Initiates resuming of previously stopped service instance. Service instance artifact could
+   * be upgraded in advance to bring some new functionality.
+   *
+   * @param fork a database view to apply arguments


Arguments are not applied to a database.

bullet-tooth added 2 commits January 21, 2020 09:11

impl services resume

1dda6cd

minor

6cf11d2

bullet-tooth requested a review from dmitry-timofeev January 21, 2020 07:18

bullet-tooth commented Jan 21, 2020

View reviewed changes

dmitry-timofeev reviewed Jan 21, 2020

View reviewed changes

dmitry-timofeev requested a review from vitvakatu January 21, 2020 11:21

add fork to the resume

4c0f684

dmitry-timofeev suggested changes Jan 22, 2020

View reviewed changes

bullet-tooth added 4 commits January 23, 2020 09:06

renaming

54b738e

Merge branch 'master' into ecr-4133

ef2c526

changed type, added tests

6a54eb3

minor

bb36ec4

bullet-tooth requested a review from dmitry-timofeev January 23, 2020 07:28

Fix the documentation

8c08540

dmitry-timofeev reviewed Jan 23, 2020

View reviewed changes

dmitry-timofeev approved these changes Jan 23, 2020

View reviewed changes

Merge branch 'master' into ecr-4133

367b32e

dmitry-timofeev merged commit eb2b5f6 into master Jan 23, 2020

dmitry-timofeev deleted the ecr-4133 branch January 23, 2020 19:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECR-4133] Add service resume #1372

[ECR-4133] Add service resume #1372

bullet-tooth commented Jan 21, 2020

bullet-tooth Jan 21, 2020

coveralls commented Jan 21, 2020 •

edited

Loading

dmitry-timofeev left a comment

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020 •

edited

Loading

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020

bullet-tooth Jan 21, 2020

dmitry-timofeev Jan 21, 2020

dmitry-timofeev Jan 21, 2020

dmitry-timofeev left a comment

dmitry-timofeev Jan 22, 2020

dmitry-timofeev Jan 22, 2020

dmitry-timofeev Jan 22, 2020

bullet-tooth Jan 22, 2020

dmitry-timofeev Jan 23, 2020

dmitry-timofeev Jan 22, 2020

dmitry-timofeev Jan 23, 2020

[ECR-4133] Add service resume #1372

[ECR-4133] Add service resume #1372

Conversation

bullet-tooth commented Jan 21, 2020

Overview

Definition of Done

Choose a reason for hiding this comment

coveralls commented Jan 21, 2020 • edited Loading

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev Jan 21, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev 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

coveralls commented Jan 21, 2020 •

edited

Loading

dmitry-timofeev Jan 21, 2020 •

edited

Loading