Add additional JVM parameters [ECR-1769] #342

vitvakatu · 2018-07-04T20:06:57Z

Overview

Add additional user-specified JVM parameters, remove debug setting.

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

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

I'd add some tests.

Also, have you explored _JAVA_OPTIONS environment variable that Travis apparently uses? I've seen message from the JVM like picked up _JAVA_OPTIONS=…. I wonder if it works in our case.

dmitry-timofeev · 2018-07-05T08:20:43Z

exonum-java-binding-core/rust/src/runtime/config.rs

+    /// Additional parameters for JVM.
+    ///
+    /// Passed directly to JVM while initializing EJB runtime.
+    /// Parameters must not have dash at the beginning.


What will happen if a user passes one? Do we check it anywhere and show a descriptive error message?

Probably good catch! In case of invalid parameter JVM will return pretty descriptive error message

It's impossible to pass a parameter with a dash (Exonum core considers it as an unknown cmd flag)

exonum-java-binding-core/rust/src/runtime/java_service_runtime.rs

@@ -58,8 +58,9 @@ impl JavaServiceRuntime {
    fn create_java_vm(config: JvmConfig) -> JavaVM {


dmitry-timofeev · 2018-07-05T08:23:42Z

exonum-java-binding-core/rust/src/runtime/config.rs

+#[derive(Debug, Clone, Copy)]
+pub enum UserParameterError {
+    /// Trying to specify a parameter that is set by EJB internally.
+    ForbiddenParameter,


How is that displayed to the user? Can we include a parameter that they shall use instead in the error message?

Good idea, I will improve this

vitvakatu · 2018-07-05T20:08:58Z

Lol. As I thought, forbidding _JAVA_OPTIONS failed service_bootstrap test on Travis.

…service # Conflicts: # exonum-java-binding-qa-service/pom.xml

# Conflicts: # exonum-java-binding-core/rust/ejb-app/start_cryptocurrency_node.sh

dmitry-timofeev · 2018-07-09T14:53:39Z

exonum-java-binding-core/rust/src/runtime/java_service_runtime.rs

-/// TODO
+/// Panics if `_JAVA_OPTIONS` environmental variable is set.
+pub fn panic_if_java_options() {
+    if env::var("_JAVA_OPTIONS").is_ok() {


I remember you wrote me there are several of them?

Actually, only _JAVA_OPTIONS is important for us, cause only this variable can overwrite command-line args

dmitry-timofeev · 2018-07-09T14:54:16Z

Marking as wip until tests are added

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

@dmitry-timofeev do we need any other tests?

dmitry-timofeev · 2018-07-16T15:12:35Z

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

@@ -7,7 +7,7 @@ use exonum::node::NodeConfig;
 use failure;
 use toml::Value;

-const EJB_DEBUG: &str = "EJB_DEBUG";
+const EJB_USER_PARAMETERS: &str = "EJB_USER_PARAMETERS";


JVM_ARGUMENTS?
(EJB_USER_PARAMETERS suggests that these are parameters of EJB).

dmitry-timofeev · 2018-07-16T15:13:41Z

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

                false,
-                "Debug mode for JVM.",
+                "Additional parameters for JVM.",


Will it be shown in help? If so, I'd document that these parameters must not have a leading dash (-), e.g., Xmx2G.

dmitry-timofeev · 2018-07-16T15:17:15Z

exonum-java-binding-core/rust/src/runtime/config.rs

+}
+
+fn check_not_forbidden(user_parameter: &str) -> Result<(), ForbiddenParameterError> {
+    if user_parameter.contains("Djava.class.path")


Isn't contains to restrictive (what if I want to specify a custom property, e.g., -Dfoo.bar=bazDjava.class.path)? Maybe starts_with?

dmitry-timofeev · 2018-07-16T15:18:09Z

exonum-java-binding-core/rust/src/runtime/config.rs

+        let validation_result = validate_and_convert("Xdebug");
+        assert_eq!(validation_result, Ok("-Xdebug".to_string()));
+
+        let validation_result = validate_and_convert("Duser.parameter");


Split into two tests to keep them focused?

dmitry-timofeev · 2018-07-16T15:21:57Z

exonum-java-binding-core/rust/src/runtime/java_service_runtime.rs

+pub fn panic_if_java_options() {
+    if env::var("_JAVA_OPTIONS").is_ok() {
+        panic!(
+            "_JAVA_OPTIONS environmental variable is set. \


environment

dmitry-timofeev · 2018-07-16T15:22:09Z

exonum-java-binding-core/rust/src/runtime/java_service_runtime.rs

+    if env::var("_JAVA_OPTIONS").is_ok() {
+        panic!(
+            "_JAVA_OPTIONS environmental variable is set. \
+             Due to the fact that it will overwrite any JVM setting, \


any JVM settings

dmitry-timofeev · 2018-07-16T15:22:24Z

exonum-java-binding-core/rust/src/runtime/java_service_runtime.rs

+             Due to the fact that it will overwrite any JVM setting, \
+             including ones set by EJB internally, this variable is \
+             forbidden for EJB applications.\n\
+             It is recommend to use `--ejb-jvm-args` command-line \


It is recommended

Ilya Bogdanov added 3 commits July 4, 2018 10:39

Add additional jvm parameters. Remove debug flag.

541307d

Fix naming and cryptocurrency script

7044e47

Fix doc comments

d53cc1d

vitvakatu self-assigned this Jul 4, 2018

vitvakatu requested a review from dmitry-timofeev July 4, 2018 20:06

Fix integration tests

c1555df

alexander-irbis approved these changes Jul 5, 2018

View reviewed changes

dmitry-timofeev reviewed Jul 5, 2018

View reviewed changes

Ilya Bogdanov added 3 commits July 5, 2018 18:35

Use ForbiddenParameterError with descriptive message

a5006fc

Forbid _JAVA_OPTIONS

20a138e

Formatting & docs

0e8d3e1

vitvakatu force-pushed the ECR-1769 branch from b114a4b to 0e8d3e1 Compare July 5, 2018 17:41

Ilya Bogdanov added 7 commits July 8, 2018 15:24

Permit _JAVA_OPTIONS for tests

8dca7a6

Fix import

935ba01

Do not panic inside JSR. Instead, panic in EJB App

abc081b

Trigger build

db24652

Merge remote-tracking branch 'remotes/upstream/master' into merge-qa-…

04539a4

…service # Conflicts: # exonum-java-binding-qa-service/pom.xml

Fix doc comment

d1a05ea

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

6ac16f2

# Conflicts: # exonum-java-binding-core/rust/ejb-app/start_cryptocurrency_node.sh

dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Jul 9, 2018

dmitry-timofeev reviewed Jul 9, 2018

View reviewed changes

Ilya Bogdanov added 3 commits July 9, 2018 19:21

Add basic tests

b9b3066

Enable rust tests

8945870

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

b96ce12

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

dmitry-timofeev reviewed Jul 16, 2018

View reviewed changes

Address review comments

2d6d239

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

a3741b3

dmitry-timofeev approved these changes Jul 16, 2018

View reviewed changes

Cargo fmt

aef3c2d

dmitry-timofeev merged commit 72fd7b1 into exonum:master Jul 17, 2018

vitvakatu deleted the ECR-1769 branch July 18, 2018 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional JVM parameters [ECR-1769] #342

Add additional JVM parameters [ECR-1769] #342

vitvakatu commented Jul 4, 2018 •

edited

Loading

dmitry-timofeev left a comment

dmitry-timofeev Jul 5, 2018

vitvakatu Jul 5, 2018

vitvakatu Jul 5, 2018

This comment was marked as resolved.

dmitry-timofeev Jul 5, 2018

vitvakatu Jul 5, 2018 •

edited

Loading

vitvakatu commented Jul 5, 2018

dmitry-timofeev Jul 9, 2018

vitvakatu Jul 9, 2018

dmitry-timofeev commented Jul 9, 2018

vitvakatu commented Jul 10, 2018

dmitry-timofeev Jul 16, 2018

dmitry-timofeev Jul 16, 2018

dmitry-timofeev Jul 16, 2018

dmitry-timofeev Jul 16, 2018

dmitry-timofeev Jul 16, 2018

dmitry-timofeev Jul 16, 2018

dmitry-timofeev Jul 16, 2018

		@@ -58,8 +58,9 @@ impl JavaServiceRuntime {
		fn create_java_vm(config: JvmConfig) -> JavaVM {

Add additional JVM parameters [ECR-1769] #342

Add additional JVM parameters [ECR-1769] #342

Conversation

vitvakatu commented Jul 4, 2018 • edited 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

This comment was marked as resolved.

Choose a reason for hiding this comment

vitvakatu Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

vitvakatu commented Jul 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev commented Jul 9, 2018

vitvakatu commented Jul 10, 2018

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 4, 2018 •

edited

Loading

vitvakatu Jul 5, 2018 •

edited

Loading