[ECR-2906] LC: Implemented System API endpoints support #716

bullet-tooth · 2019-02-15T12:32:35Z

Overview

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

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 · 2019-02-15T12:33:47Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+   * @throws RuntimeException if the client is unable to complete a request
+   *        (e.g., in case of connectivity problems)
+   */
+  boolean healthCheck();


more proper name?

isNodeConnected? isNodeInNetwork?

BTW, is it OK to get an exception if the client is not connected? (probably, yes, as we need to communicate different problems differently; but the helthCheck is kind of an innocent name)

bullet-tooth · 2019-02-15T12:34:21Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+
+  /**
+   * Returns <b>true</b> if the node is connected to the other peers.
+   * And <b>false</b> otherwise.


is it required to write at each method that it works in a blocking way? 🤔

I'd put that in a class level comment (unless we make a hybrid with both blocking and non-blocking APIs)

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

exonum-light-client/src/main/java/com/exonum/client/HealthCheckResponse.java

This reverts commit c5fd9f6

coveralls · 2019-02-15T14:13:17Z

Coverage increased (+0.002%) to 85.53% when pulling 6562719 on ecr-2906 into b0c0d9c on master.

dmitry-timofeev

Good, some naming/documentation suggestions.

dmitry-timofeev · 2019-02-18T10:36:56Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java


+  /**
+   * Returns a number of unconfirmed transactions those are currently located in
+   * the memory pool and are waiting for acceptance to a block.


Nit: AFAIK, it is not a memory pool (they are persisted to avoid their loss in case a node goes down).

dmitry-timofeev · 2019-02-18T10:37:10Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

  HashCode submitTransaction(TransactionMessage tx);

+  /**
+   * Returns a number of unconfirmed transactions those are currently located in


that/which are currently …

dmitry-timofeev · 2019-02-18T10:38:43Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+
+  /**
+   * Returns <b>true</b> if the node is connected to the other peers.
+   * And <b>false</b> otherwise.


I'd put that in a class level comment (unless we make a hybrid with both blocking and non-blocking APIs)

dmitry-timofeev · 2019-02-18T10:39:37Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+  int getUnconfirmedTransactions();
+
+  /**
+   * Returns <b>true</b> if the node is connected to the other peers.


I'd not use bold for that, but no formatting or monospace.

dmitry-timofeev · 2019-02-18T10:40:41Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+
+  /**
+   * Returns <b>true</b> if the node is connected to the other peers.
+   * And <b>false</b> otherwise.


I'd consider simplifying to:

to the other peers; false — otherwise.

dmitry-timofeev · 2019-02-18T10:42:37Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+   * @throws RuntimeException if the client is unable to complete a request
+   *        (e.g., in case of connectivity problems)
+   */
+  boolean healthCheck();


isNodeConnected? isNodeInNetwork?

BTW, is it OK to get an exception if the client is not connected? (probably, yes, as we need to communicate different problems differently; but the helthCheck is kind of an innocent name)

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

dmitry-timofeev · 2019-02-18T10:46:11Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+  int getUnconfirmedTransactions();
+
+  /**
+   * Returns <b>true</b> if the node is connected to the other peers.


Shall it be connected to all peers to be in the network? Or some peers? Or at least one peer?

dmitry-timofeev · 2019-02-18T10:49:30Z

exonum-light-client/src/test/java/com/exonum/client/ExonumHttpClientIntegrationTest.java

+  @Test
+  void getUserAgentInfo() throws InterruptedException {
+    // Mock response
+    String mockResponse = "exonum 0.6.0/rustc 1.26.0 (2789b067d 2018-03-06)\n\n/Mac OS10.13.3";


I see — a String will do 🙃

dmitry-timofeev · 2019-02-18T10:52:12Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

+   * @throws RuntimeException if the client is unable to complete a request
+   *        (e.g., in case of connectivity problems)
+   */
+  int getUnconfirmedTransactions();


Will such a name work well when we add getTransaction/getTransactions, as it does not return transactions, but their number? getNumUnconfirmedTransactions — will be more accurate, but I am not quite happy with the name length :-)

dmitry-timofeev · 2019-02-18T15:46:20Z

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

 * Main interface for Exonum Light client.
 * Provides a convenient way for interaction with Exonum framework APIs.
+ * All the methods of the interface work in a blocking way
+ * i.e. invoke underlying request immediately, and blocks until the response can be processed


… and block until … or there is an error/an error occurs.

added support for Exonum v10

dmitry-timofeev · 2019-02-19T11:52:16Z

exonum-light-client/src/main/java/com/exonum/client/response/ConsensusStatus.java

-class HealthCheckResponse {
-  boolean connectivity;
+public enum ConsensusStatus {
+  ACTIVE,


Do you happen to know the difference between these two? As it is public, I'd document it.

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java

dmitry-timofeev · 2019-02-19T11:56:57Z

exonum-light-client/src/main/java/com/exonum/client/response/HealthCheckInfo.java

-  int size;
+public class HealthCheckInfo {
+  ConsensusStatus consensusStatus;
+  int connectionsNumber;


Shall we enforce any restrictions on the values?

do you mean negative values? Also, we have some restriction for a number of network nodes. But I'm not sure that LC should know about theoretical maximum of that count

Yeah, I though mostly of that, because there might not be more elaborate (e.g., if the consensus is enabled, might we have any connections — I have no idea). But I agree it is not required to put here checks of maximum values (and we can skip negatives too)

dmitry-timofeev · 2019-02-19T11:57:43Z

exonum-light-client/src/test/java/com/exonum/client/SystemApiHelperTest.java

+    return ImmutableList.of(
+        Arguments.of("{\"consensus_status\": \"Enabled\", \"connectivity\": \"NotConnected\"}",
+            new HealthCheckInfo(ConsensusStatus.ENABLED, 0)),
+        Arguments.of("{\"consensus_status\": \"Disabled\", \"connectivity\": \"NotConnected\"}",


Do you think it makes sense to request the core to use a consistent format? If so, please submit an issue.

created ECR-2925

dmitry-timofeev · 2019-02-19T13:55:39Z

exonum-light-client/src/main/java/com/exonum/client/response/ConsensusStatus.java

 package com.exonum.client.response;

+/**
+ * Consensus status.


of a particular node? of the whole network?

dmitry-timofeev · 2019-02-19T13:57:46Z

exonum-light-client/src/main/java/com/exonum/client/response/HealthCheckInfo.java

-  int size;
+public class HealthCheckInfo {
+  ConsensusStatus consensusStatus;
+  int connectionsNumber;


Yeah, I though mostly of that, because there might not be more elaborate (e.g., if the consensus is enabled, might we have any connections — I have no idea). But I agree it is not required to put here checks of maximum values (and we can skip negatives too)

dmitry-timofeev · 2019-02-19T14:01:31Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

+   * Json object wrapper for submit transaction response.
+   */
+  @Value
+  class SubmitTxResponse {


Shan't it be static as aboe? Or is it by default?

dmitry-timofeev · 2019-02-19T14:02:57Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

+    return json().toJson(request);
+  }
+
+  static HashCode submitTxParser(String json) {


+parseSubmitTxResponse(String response) (so that it is a verb describing the action, otherwise it sounds as it returns some kind of a parser)?

dmitry-timofeev · 2019-02-19T14:03:36Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

+ */
+final class ExplorerApiHelper {
+
+  static String submitTxBody(String message) {


+createSubmitTxBody(String txMessage) 🔽 ?

dmitry-timofeev · 2019-02-19T15:43:23Z

Awesome, let’s push it 💯

Implemented System API endpoints support

0ebdd95

bullet-tooth requested review from dip56, MakarovS and dmitry-timofeev February 15, 2019 12:32

bullet-tooth commented Feb 15, 2019

View reviewed changes

exonum-light-client/src/main/java/com/exonum/client/ExonumClient.java Show resolved Hide resolved

bullet-tooth commented Feb 15, 2019

View reviewed changes

exonum-light-client/src/main/java/com/exonum/client/HealthCheckResponse.java Outdated Show resolved Hide resolved

bullet-tooth added 4 commits February 15, 2019 14:42

test rename

7b33137

fmt

7c6d080

fmt

c5fd9f6

Revert lombok changes incidentally committed

80334bc

This reverts commit c5fd9f6

added javadocs

411a1a6

dmitry-timofeev suggested changes Feb 18, 2019

View reviewed changes

bullet-tooth added 4 commits February 18, 2019 13:44

Merge branch 'master' into ecr-2906

c7746a4

fixed PR comments

0aae6e6

fix build

e0db8ff

Merge branch 'master' into ecr-2906

688f924

dmitry-timofeev reviewed Feb 18, 2019

View reviewed changes

bullet-tooth added 3 commits February 19, 2019 09:07

fix javadocs

a97222c

Merge remote-tracking branch 'origin/ecr-2906' into ecr-2906

e4a4969

revert user agent response parsing

515bc2c

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

Fixed PR comments

6d819b2

added support for Exonum v10

dmitry-timofeev suggested changes Feb 19, 2019

View reviewed changes

bullet-tooth added 3 commits February 19, 2019 14:02

helpers composition

8ab36f1

javadoc update

0e5dc5c

added todo

826c351

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

Merge branch 'master' into ecr-2906

809be09

dmitry-timofeev reviewed Feb 19, 2019

View reviewed changes

dmitry-timofeev approved these changes Feb 19, 2019

View reviewed changes

Fix PR comments

6562719

dmitry-timofeev approved these changes Feb 19, 2019

View reviewed changes

dmitry-timofeev merged commit 9e0a3dd into master Feb 19, 2019

dmitry-timofeev deleted the ecr-2906 branch February 19, 2019 15:44

[ECR-2906] LC: Implemented System API endpoints support #716

[ECR-2906] LC: Implemented System API endpoints support #716

Uh oh!

Conversation

bullet-tooth commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

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!

Uh oh!

Uh oh!

coveralls commented Feb 15, 2019 • 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!

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!

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!

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 Feb 19, 2019

Uh oh!

Uh oh!

bullet-tooth commented Feb 15, 2019 •

edited

Loading

coveralls commented Feb 15, 2019 •

edited

Loading