[ECR-4133] Add service resume by bullet-tooth · Pull Request #1372 · exonum/exonum-java-binding

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

+   *     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

  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

  }

+  /**
+   * 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

+   *     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

+   *
+   * @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

+   *     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

+      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

+  /** 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


  @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

+    // 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

  "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

+   * 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

+   * <!--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

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


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

+   * 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

Conversation

bullet-tooth commented Jan 21, 2020

Overview

Definition of Done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

dmitry-timofeev Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

coveralls commented Jan 21, 2020 •

edited

Loading

dmitry-timofeev Jan 21, 2020 •

edited

Loading