[ECR-4104] Allow services stopping #1358

bullet-tooth · 2020-01-13T08:51:09Z

Overview

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

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-13T13:24:02Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/transport/VertxServer.java

+      checkNotStopped();
+      rootRouter.getRoutes()
+          .stream()
+          .filter(r -> r.getPath().equals(mountPoint))


looks like it should work well because service paths are unique within the network by design

coveralls · 2020-01-13T13:41:13Z

Coverage increased (+0.1%) to 85.824% when pulling 6537e25 on ecr-4104 into 7608a6d on master.

dmitry-timofeev · 2020-01-14T11:59:11Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/transport/Server.java

   */
  void mountSubRouter(String mountPoint, Router subRouter);

+  void removeSubRouter(String mountPoint);


Specification.

dmitry-timofeev · 2020-01-14T11:59:36Z

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

+        activateService(instanceSpec);
+      } else if (instanceStatus.equals(Status.STOPPED)) {
+        stopService(instanceSpec);
+      } else if (instanceStatus.equals(Status.NONE)) {


What's the point of notifying of NONE status? What does it mean?

None status means that the service is added but not active yet. Such could happen when restoring instances state.
See: exonum/src/runtime/dispatcher/mod.rs:263

Runtime#update_service_status does not have NONE status. It accepts a
status: InstanceStatus which is either active or stopped (at our revision):

https://github.com/exonum/exonum/blob/3c5bf23c090c6a6cc8ae184ce18f1cab34ac2fc0/exonum/src/runtime/types.rs#L430-L437

Also it could have a None rust type. See the mapping to protobuf msg: https://github.com/exonum/exonum/blob/3c5bf23c090c6a6cc8ae184ce18f1cab34ac2fc0/exonum/src/runtime/types.rs#L461

Runtime#update_service_status cannot receive NONE as InstanceStatus, so it's not the case. AFAIK NONE is only used by internal dispatcher logic as a temporary value.

There is no Option in the Runtime#update_service_status API.

You are right, got it. Dispatcher will panic in case of Node during restore.

dmitry-timofeev · 2020-01-14T12:00:43Z

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

+      } else if (instanceStatus.equals(Status.STOPPED)) {
+        stopService(instanceSpec);
+      } else if (instanceStatus.equals(Status.NONE)) {
+        logger.warn("None status for the service instance {}, ignore. "


log at info? Also, I am not sure if such questions shall appear in the log (it would be nice if we — as SR developers — do know for sure):
"Possibly restoring services state after reboot?"

What does Runtime spec say?

Core Dispatcher invokes updateInstanceStatus for each service in the schema regardless of the status. Not sure why do we restore not active services?

Re none — see the reply above; re stopped — are you saying the core confuses a "request to stop a service instance" with a "notification that a service instance has been stopped already"? Why is that?

are you saying the core confuses a "request to stop a service instance" with a "notification that a service instance has been stopped already"? Why is that?

@bullet-tooth Have you managed to clarify that? Shall the statuses in core be improved to avoid decoding of what 'stopped' means in each runtime?

Created a ticket https://jira.bf.local/browse/ECR-4128

dmitry-timofeev · 2020-01-14T12:01:31Z

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


-  private ServiceWrapper createService(ServiceInstanceSpec instanceSpec) {
+  private void stopService(ServiceInstanceSpec instanceSpec) {
+    // TODO: ECR-2334 add restriction for creation new snapshots


https://jira.bf.local/browse/ECR-2334?focusedCommentId=73307&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-73307

dmitry-timofeev · 2020-01-14T12:02:13Z

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

+  private void stopService(ServiceInstanceSpec instanceSpec) {
+    // TODO: ECR-2334 add restriction for creation new snapshots
+    String name = instanceSpec.getName();
+    Optional<ServiceWrapper> activeService = findService(name);


Why does this code perform a no-op when stopping of a non-existent service
is requested? Shan't it just throw? In this case, a simpler

int id = instanceSpec.getId(); ServiceWrapper service = getServiceById(id); // Will throw if does not exist.

will do.

we will have non-existent service in case of restoring state

Interesting — I'd consider if any changes to how core works are needed, if we don't miss anything: #1358 (comment)

dmitry-timofeev · 2020-01-14T12:02:51Z

...binding/core/src/test/java/com/exonum/binding/core/transport/VertxServerIntegrationTest.java

+
+      server.mountSubRouter(path, server.createRouter());
+      Assertions.assertThat(server.getMountedRoutes())
+          .anyMatch(route -> routPathEquals(route, path));


dmitry-timofeev · 2020-01-14T12:03:49Z

...binding/core/src/test/java/com/exonum/binding/core/transport/VertxServerIntegrationTest.java

  }

+  @ParameterizedTest
+  @ValueSource(strings = {"/foo", "/foo/", "/foo/bar", "/foo/:bar", "/foo/:bar/"})


Are ":bar" 📊 request params?

Yes, it's request param.

vitvakatu · 2020-01-15T06:43:08Z

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

+   * Activation leads to the service instance registration, allowing subsequent operations on it:
   * transactions, API requests.
+   * Stopping leads to the service disabling i.e. stopped service does not execute transactions,
+   * process events, provide APIs, etc. And it becomes unavailable to other services,


The latest sentence is rather unclear. What becomes unavailable? I thought database indexes are still available after service stopping. Service interface methods (i.e. transactions) becomes unavailable, but we do not have support for them in EJB.

dmitry-timofeev · 2020-01-15T11:04:41Z

Also, ECR-4119.

dmitry-timofeev

The code looks good, but I suggest to clarify the question on the stopped status (#1358 (comment)).

dmitry-timofeev · 2020-01-16T16:07:23Z

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

+      if (instanceStatus.equals(Status.ACTIVE)) {
+        activateService(instanceSpec);
+      } else if (instanceStatus.equals(Status.STOPPED)) {
+        stopService(instanceSpec);
+      } else {
+        String msg = String.format("Unexpected status %s received for the service %s",
+            instanceStatus.name(), instanceSpec.getName());
+        logger.error(msg);
+        throw new IllegalArgumentException(msg);
      }


I'd suggest a switch:

switch (instanceStatus) { case ACTIVE: activateService(instanceSpec); break; case STOPPED: stopService(instanceSpec); break; default: String msg = String.format("Unexpected status %s received for the service %s", instanceStatus.name(), instanceSpec.getName()); logger.error(msg); throw new IllegalArgumentException(msg); }

dmitry-timofeev · 2020-01-17T09:29:37Z

I think we may integrate it now and adjust after ECR-4128. However, the build is red.

bullet-tooth · 2020-01-17T13:34:52Z

I think we may integrate it now and adjust after ECR-4128. However, the build is red.

green :)

impl services stopping

fcc3ba2

bullet-tooth added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jan 13, 2020

bullet-tooth added 2 commits January 13, 2020 14:59

Merge branch 'master' into ecr-4104

7ddac54

imp stopping

d436f0b

bullet-tooth commented Jan 13, 2020

View reviewed changes

minor

9203695

bullet-tooth removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jan 13, 2020

bullet-tooth requested a review from dmitry-timofeev January 13, 2020 13:24

added todo

4ca1ca9

dmitry-timofeev reviewed Jan 14, 2020

View reviewed changes

fir PR comments

01aaea6

bullet-tooth mentioned this pull request Jan 14, 2020

[ECR-2334]: Using different Node instances in services #1365

Merged

7 tasks

vitvakatu reviewed Jan 15, 2020

View reviewed changes

bullet-tooth added 2 commits January 16, 2020 12:41

Rm None status

8c99ae8

Merge branch 'master' into ecr-4104

c2534de

bullet-tooth requested review from vitvakatu and dmitry-timofeev January 16, 2020 10:43

Merge branch 'master' into ecr-4104

a80430a

dmitry-timofeev reviewed Jan 16, 2020

View reviewed changes

minor fix

289804f

bullet-tooth added 3 commits January 17, 2020 12:28

fix checkstyle

4d6d0a4

Merge branch 'master' into ecr-4104

97828cd

Merge branch 'master' into ecr-4104

6537e25

dmitry-timofeev approved these changes Jan 17, 2020

View reviewed changes

dmitry-timofeev merged commit 6394430 into master Jan 17, 2020

dmitry-timofeev deleted the ecr-4104 branch January 17, 2020 13:47

dmitry-timofeev restored the ecr-4104 branch January 17, 2020 13:47

[ECR-4104] Allow services stopping #1358

[ECR-4104] Allow services stopping #1358

Uh oh!

Conversation

bullet-tooth commented Jan 13, 2020

Overview

Definition of Done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

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 commented Jan 15, 2020

Uh oh!

dmitry-timofeev left a comment • 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!

dmitry-timofeev commented Jan 17, 2020

Uh oh!

bullet-tooth commented Jan 17, 2020

Uh oh!

Uh oh!

coveralls commented Jan 13, 2020 •

edited

Loading

dmitry-timofeev left a comment •

edited

Loading