Implement run-dev command support [ECR-3727] by vitvakatu · Pull Request #1217 · exonum/exonum-java-binding

vitvakatu · 2019-11-06T16:39:14Z

Overview

Also fixed bug in java_runtime_factory module and fixed service_runtime_bootstrap native IT (it is still disabled)

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

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

coveralls · 2019-11-07T08:00:50Z

Coverage remained the same at 85.602% when pulling 6711f78 on vitvakatu:ecr-3727-run-dev-support into f74692b on exonum:master.

dmitry-timofeev

I have a couple of questions to the intended behaviour:

Is this command expected to always start with a fresh blockchain? I.e., once the node is stopped, can the db be scrapped?
If it is, then why doesn't it use a new temporary directory to store the database with auto-delete on exit feature? Deleting a directory that this process has created is fine, a directory that was previously created and (a) might not contain a database but user files; (b) might belong to another running process is not.
If it isn't, then shan't we use a new temp by default, but allow specifying an existing db directory?
A related question — if I started with a temporary directory, would I ever want to keep the db after I shutdown the node?

dmitry-timofeev · 2019-11-07T10:59:56Z

+    /// Run the node in development mode (generate configuration and db files automatically).
+    ///
+    /// Runs one node with public API address 127.0.0.1:8080, private API address 127.0.0.1:8081,
+    /// EJB port 6400 and no logging configuration.


no or default?

Initially no way to specify Java logging configuration. Now added a special option for it. Not sure if I need to duplicate all other ejb parameters.

I didn't ask for an option to add configuration, I asked whether such invocation will have no configuration or the default. I do not expect and consider as sensible supporting logging configuration for this command.

vitvakatu · 2019-11-11T16:08:16Z

Implementation has been rewritten almost completely, now it supports setting database directory and node configuration file.

vitvakatu · 2019-11-13T06:31:34Z

In current implementation, user is allowed to provide the following parameters to the CLI:

artifacts-path - path to a directory containing Java artifacts. Works exactly as run command
db-path - path to a directory for database files. If not provided, system temporary directory is used. No files are deleted automatically, so the user must clean them manually if needed.
node-config - path to a node configuration file. If not provided, defaults to db_path/config and new config will be generated before node start. Again, no files are deleted automatically. Existing node configuration is used.
ejb-log-config-path - same as for run command.

So the user can:

Easily run a node with "default" settings.
Run node with specific database and/or node configuration.
Reuse database and/or node configuration from previous runs

dmitry-timofeev

I doubt it works, and the first question on the intended behaviour remains: is it expected to always start anew? Do we need to support these four options?

dmitry-timofeev · 2019-11-13T11:09:16Z

+    ///
+    /// If <db_path> parameter is provided, returns <db_path>/config. Else returns system temporary
+    /// directory.
+    fn config_directory(&self) -> Result<PathBuf, failure::Error> {


It is named as a getter but it will create a new temporary directory if none exists.

dmitry-timofeev · 2019-11-13T11:11:20Z

+        let node_config_path = concat_path(config_directory.clone(), "node.toml");
+
+        // Configuration files exist, skip generation.
+        if config_directory.exists() {


How does existence of a config_directory (a temp dir may be created in config_directory) imply config file existence? Have you tested this?

Error messages when node configuration file is deleted from database_path/config directory:

Error: Os { code: 2, kind: NotFound, message: "No such file or directory" } loading config from testnet/config/node.toml

dmitry-timofeev · 2019-11-13T11:13:28Z

+///   files and <tmp>/config for node configuration files.
+/// 2. Only `<db_path>` is provided. `<db_path>` is used for database files, `<db_path>/config` is
+///   used for configuration files. If `<db_path>/config` exists, existing configuration files are
+///   used, otherwise node configuration is auto-generated.


Auto-generated or extracted from the database? If the first, would it even work?

vitvakatu · 2019-11-14T07:49:22Z

Reimplemented once more. Now the user must provide a required blockchain-path parameter which sets the directory used to store both database files and node configuration files. User must manually delete this directory (or use the new one) if a new node needs to be started.

dmitry-timofeev

Good, thanks!

To give some context: we discussed that the combinations of parameters when there are separate db path and config path and they are optional are too complex to implement and document and maintain; and unclear how useful; making the blockchain path in tmp would entail some restrictions and extra burden on the user since there is no easy way to implement auto-cleaning of that directory after node finishes (as a restriction, don't use in long-running systems that generate some transactions as nothing will clear the database directory until reboot). If we consider that tmp-that-is-not-cleaned is fine for the target usages, or tmp-that-is-cleaned is required (the use cases justify the complexity) — we can add any of that later, when there is demand.

dmitry-timofeev · 2019-11-14T18:37:22Z

+    /// Database is located in <blockchain_path>/db directory, node configuration files
+    /// are located in <blockchain_path>/config directory. Existing files and directories are
+    /// reused. To generate new node configuration and start a new blockchain, the user must
+    /// manually delete existing <blockchain_path> directory.


or specify a new one.

dmitry-timofeev · 2019-11-14T18:57:37Z

The clippy is unhappy.

skletsun

LGTM

Ilya Bogdanov added 2 commits November 6, 2019 19:37

Implement run-dev command for EJB App

942dfcf

Fix service runtime bootstrap IT

b970a19

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

Add basic docs for run-dev command

634497c

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

vitvakatu requested review from dmitry-timofeev and skletsun November 7, 2019 07:38

dmitry-timofeev reviewed Nov 7, 2019

View reviewed changes

Comment thread exonum-java-binding/core/rust/src/runtime/cmd.rs Outdated

Ilya Bogdanov added 4 commits November 11, 2019 10:33

Revert changes to service_runtime_bootstrap

7f34ddb

Reimplement run-dev command

1ca71fa

Merge branch 'master' into ecr-3727-run-dev-support

aec25e1

Add logging configuration & fix documentation

bcc9b82

Ilya Bogdanov added 2 commits November 12, 2019 14:36

Fix clippy issue

2128f86

Merge branch 'master' into ecr-3727-run-dev-support

c4ae39b

dmitry-timofeev suggested changes Nov 13, 2019

View reviewed changes

dmitry-timofeev reviewed Nov 13, 2019

View reviewed changes

Comment thread exonum-java-binding/core/rust/src/runtime/cmd.rs Outdated

Ilya Bogdanov added 3 commits November 14, 2019 10:36

Use simpler approach to run-dev parameters

64113e8

Small fixes

4a87349

Revert changes in service_runtime_bootstrap test

679413e

dmitry-timofeev approved these changes Nov 14, 2019

View reviewed changes

dmitry-timofeev requested a review from ipenkin November 14, 2019 18:58

Ilya Bogdanov added 2 commits November 15, 2019 11:12

Tiny fix to the docs

b4ca5b2

Fix clippy issue

c5ee370

dmitry-timofeev approved these changes Nov 15, 2019

View reviewed changes

Merge branch 'master' into ecr-3727-run-dev-support

6711f78

skletsun approved these changes Nov 15, 2019

View reviewed changes

dmitry-timofeev merged commit f20fe67 into exonum:master Nov 15, 2019

Conversation

vitvakatu commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

coveralls commented Nov 7, 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!

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!

Uh oh!

Uh oh!

Uh oh!

vitvakatu commented Nov 11, 2019

Uh oh!

vitvakatu commented Nov 13, 2019

Uh oh!

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vitvakatu commented Nov 14, 2019

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!

dmitry-timofeev commented Nov 14, 2019

Uh oh!

skletsun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

vitvakatu commented Nov 6, 2019 •

edited

Loading

coveralls commented Nov 7, 2019 •

edited

Loading