Add list of services to enable (ECR-1766) & bundle btc-anchoring (ECR-1366) #338

vitvakatu · 2018-07-04T07:55:03Z

Overview

Added list of services to enable in a separate TOML file.
Now it supports two services - configuration and anchoring.

See: https://jira.bf.local/browse/ECR-1766
See: https://jira.bf.local/browse/ECR-1366

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
The continuous integration build passes

dmitry-timofeev

Good, I'd add some tests.

dmitry-timofeev · 2018-07-04T09:40:00Z

exonum-java-binding-core/rust/ejb-app/src/main.rs

+    let mut builder =
+        fabric::NodeBuilder::new().with_service(Box::new(java_bindings::JavaServiceFactory));
+    for service in &services.services {
+        if service == CONFIGURATION_SERVICE {


Can we have a map of system service factories (name -> factory)? Then we can get rid of this chain of if/else if/…/else, which will grow a little:

for service_name in service_to_enable: if (system_services.contains(service_name)): builder = builder.with_service(system_services.get(service_name)); else: panic("unknown service: " + service_name);

You can even put there a java service factory, and append its name to the list of service-to-enable.

Okay, will try

dmitry-timofeev · 2018-07-04T09:41:37Z

exonum-java-binding-core/rust/ejb-app/src/main.rs

 use java_bindings::exonum::helpers::fabric;

+const PATH_TO_SERVICES_TO_ENABLE: &str = "ejb_app_services.toml";


Is it expected to be in the current working directory?

dmitry-timofeev · 2018-07-04T09:42:16Z

exonum-java-binding-core/rust/ejb-app/ejb_app_services.toml

@@ -0,0 +1 @@
+services = ["configuration"]


Why this file is checked in?

Because configuration service should be enabled by default. I can remove this file though

But if that's the case it shall work reliably with empty or no file?

Oh well... 🤔
Indeed, we don't need this file.

dmitry-timofeev · 2018-07-05T07:35:50Z

exonum-java-binding-core/rust/ejb-app/src/main.rs

+    );
+    service_factories.insert(
+        BTC_ANCHORING_SERVICE.to_owned(),
+        Box::new(BtcAnchoringServiceFactory) as Box<ServiceFactory>,


Is it OK to instantiate the factories here (instead of providing a factory method for a factory — a method reference)?

Yes, it's okay

dmitry-timofeev · 2018-07-05T07:42:32Z

exonum-java-binding-core/rust/ejb-app/src/main.rs

+
+    let mut builder = fabric::NodeBuilder::new();
+    for service_name in &services {
+        match service_factories.remove(service_name) {


I think of this map of factories as an immutable map, but see it's not the case. I also see a new one is created when service_factories is called, so it’s OK in terms of correctness to remove the factories from it. This process looks a little complicated. Is there a good reason to not use an immutable map?

Immutable map will give only references to values to me which is not enough - I need owned values. Well, technically, we can use closures stored in HashMap. Will try and look at complexity.
To be honest, the current solution is much more complicated then first attempt with simple if...else

To be honest, the current solution is much more complicated

Yes, it is more complex. Rust complicated things :-) If/else is repetitive and error prone (you have to test each branch basically), but if a solution with a map results in complex Rust code, we can go back to it.

alexander-irbis · 2018-07-05T13:42:30Z

exonum-java-binding-core/rust/ejb-app/src/main.rs

+    services: HashSet<String>,
+}
+
+fn service_factories() -> HashMap<String, Box<ServiceFactory + 'static>> {


Isn't Box<ServiceFactory> 'static by default?

Should be, but compiler doesn't think so 🙃

vitvakatu · 2018-07-05T15:06:07Z

@dmitry-timofeev I tried several approaches. The current one is the most simple and effective one. Additionally, it's just a temporary solution for our problems.

vitvakatu · 2018-07-05T15:08:10Z

@alexander-irbis oh lol. Just tried to remove 'static bound and it works! Don't know why I had compilation error before

dmitry-timofeev · 2018-07-05T15:33:47Z

@dmitry-timofeev I tried several approaches. The current one is the most simple and effective one.

Looks good, please add tests.

dmitry-timofeev · 2018-07-06T10:20:14Z

exonum-java-binding-core/rust/ejb-app/src/node_builder.rs

+
+    #[test]
+    fn broken_config() {
+        let cfg = create_config("broken.toml", "not_list = 1");


Shan't we throw in this case?

Hmmm... No, be just use default config... I'll consider panicing

dmitry-timofeev · 2018-07-06T10:21:16Z

run_ejb_app_tests.sh

+export LD_LIBRARY_PATH="$(find ${JAVA_HOME} -type f -name libjvm.* | xargs -n1 dirname)"
+echo "LD_LIBRARY_PATH=${LD_LIBRARY_PATH}"
+
+# Compile all Java modules by default to ensure that ejb-fakes module, which is required


Why, do app tests depend on any Java classes?

dmitry-timofeev · 2018-07-06T10:22:08Z

run_ejb_app_tests.sh

@@ -0,0 +1,41 @@
+#!/usr/bin/env bash
+# Runs EJB App tests (ejb-core/rust/ejb-app).


Why don't we call run_ejb_app_tests.sh from ~~run_all_tests.sh~~ run_native_integration_tests.sh?

Edit: I see, we do call from run_all_tests. Does it belong there or in native_its?

I think it's just separated tests, not native_its

dmitry-timofeev · 2018-07-06T13:10:31Z

exonum-java-binding-core/rust/ejb-app/src/node_builder.rs

-    });
+    use std::fs::File;
+    use std::io::Read;
+    let mut services = if let Ok(mut file) = File::open(path) {


I'd consider adding a comment to express the intent.

alexander-irbis · 2018-07-06T15:29:31Z

exonum-java-binding-core/rust/ejb-app/src/node_builder.rs

+    fn broken_config() {
+        let cfg = create_config("broken.toml", "not_list = 1");
+        let services_to_enable = services_to_enable(cfg);
+        assert_eq!(services_to_enable.len(), 2);


Perhaps there is no need in asserts?

dmitry-timofeev · 2018-07-07T12:50:41Z

exonum-java-binding-core/rust/ejb-app/src/node_builder.rs

@@ -118,9 +118,6 @@ mod tests {
    fn broken_config() {
        let cfg = create_config("broken.toml", "not_list = 1");
        let services_to_enable = services_to_enable(cfg);


Unused variable seems to break clippy (shall it be prefixed with an underscore?).

dmitry-timofeev · 2018-07-07T13:30:59Z

@vitvakatu , would you please update the instructions in TUTORIAL.md, as a separate task?

Ilya Bogdanov added 4 commits July 4, 2018 00:09

Add list of services to enable [ECR-1766]

7a03275

Bundle anchoring service into EJB App [ECR-1366]

2017463

Use constants

0a5cbe2

Fix script & newlines

ee5d386

vitvakatu self-assigned this Jul 4, 2018

vitvakatu requested review from alexander-irbis and dmitry-timofeev July 4, 2018 07:55

dmitry-timofeev reviewed Jul 4, 2018

View reviewed changes

dmitry-timofeev requested a review from stanislav-tkach July 4, 2018 11:41

Ilya Bogdanov added 2 commits July 4, 2018 15:53

Massive refactoring

b7972b3

Remove ejb_app_services file

f1d1410

dmitry-timofeev reviewed Jul 5, 2018

View reviewed changes

alexander-irbis approved these changes Jul 5, 2018

View reviewed changes

Ilya Bogdanov added 2 commits July 5, 2018 18:06

Merge remote-tracking branch 'remotes/upstream/master' into ECR-1766

12ba83d

Remove static bound

7c1e73d

Ilya Bogdanov added 4 commits July 5, 2018 21:08

Move implementation to separate module

c0e0ccc

Split implementation to functions

da8f28d

Add tests

d7137e3

Run EJB App tests on Travis

99554b9

dmitry-timofeev reviewed Jul 6, 2018

View reviewed changes

Fail if invalid config

112bf35

dmitry-timofeev approved these changes Jul 6, 2018

View reviewed changes

Add comment

ac14ae6

dmitry-timofeev approved these changes Jul 6, 2018

View reviewed changes

Merge branch 'master' into ECR-1766

d96d178

dmitry-timofeev added the Next to merge 🛬 The next PR to merge (that is up-to-date with master) label Jul 6, 2018

alexander-irbis approved these changes Jul 6, 2018

View reviewed changes

Fix tests & remove invalid comment

de86ec1

dmitry-timofeev approved these changes Jul 7, 2018

View reviewed changes

dmitry-timofeev reviewed Jul 7, 2018

View reviewed changes

Fix unused variable

6ac9526

dmitry-timofeev approved these changes Jul 7, 2018

View reviewed changes

dmitry-timofeev merged commit 5af6dca into exonum:master Jul 7, 2018

vitvakatu deleted the ECR-1766 branch July 7, 2018 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add list of services to enable (ECR-1766) & bundle btc-anchoring (ECR-1366) #338

Add list of services to enable (ECR-1766) & bundle btc-anchoring (ECR-1366) #338

vitvakatu commented Jul 4, 2018 •

edited by dmitry-timofeev

Loading

dmitry-timofeev left a comment

dmitry-timofeev Jul 4, 2018

vitvakatu Jul 4, 2018

dmitry-timofeev Jul 4, 2018

vitvakatu Jul 4, 2018

dmitry-timofeev Jul 4, 2018

vitvakatu Jul 4, 2018

dmitry-timofeev Jul 4, 2018

vitvakatu Jul 4, 2018

dmitry-timofeev Jul 5, 2018

vitvakatu Jul 5, 2018

dmitry-timofeev Jul 5, 2018

vitvakatu Jul 5, 2018

dmitry-timofeev Jul 5, 2018

alexander-irbis Jul 5, 2018

vitvakatu Jul 5, 2018

vitvakatu commented Jul 5, 2018

vitvakatu commented Jul 5, 2018

dmitry-timofeev commented Jul 5, 2018

dmitry-timofeev Jul 6, 2018

vitvakatu Jul 6, 2018

dmitry-timofeev Jul 6, 2018 •

edited

Loading

dmitry-timofeev Jul 6, 2018 •

edited

Loading

vitvakatu Jul 6, 2018

dmitry-timofeev Jul 6, 2018

alexander-irbis Jul 6, 2018

dmitry-timofeev Jul 7, 2018

dmitry-timofeev commented Jul 7, 2018

		use java_bindings::exonum::helpers::fabric;

		const PATH_TO_SERVICES_TO_ENABLE: &str = "ejb_app_services.toml";

		@@ -0,0 +1,41 @@
		#!/usr/bin/env bash
		# Runs EJB App tests (ejb-core/rust/ejb-app).

Add list of services to enable (ECR-1766) & bundle btc-anchoring (ECR-1366) #338

Add list of services to enable (ECR-1766) & bundle btc-anchoring (ECR-1366) #338

Conversation

vitvakatu commented Jul 4, 2018 • edited by dmitry-timofeev Loading

Overview

Definition of Done

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

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

vitvakatu commented Jul 5, 2018

vitvakatu commented Jul 5, 2018

dmitry-timofeev commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

dmitry-timofeev Jul 6, 2018 • 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

dmitry-timofeev commented Jul 7, 2018

vitvakatu commented Jul 4, 2018 •

edited by dmitry-timofeev

Loading

dmitry-timofeev Jul 6, 2018 •

edited

Loading

dmitry-timofeev Jul 6, 2018 •

edited

Loading