IGNITE-23196 [ducktests] Change JVM options to JDK11 ones #11536

okreda1 · 2024-09-16T15:11:49Z

changed default settings for gc log (-Xloggc ->-Xlog:gc)
added new logs for safepoints
added new variable to disable safepoints log
removed UnlockCommercialFeatures because it was used only for jfr, but no longer needed for openjdk11
changed option in check_boolean_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt to FlightRecorder
changed option in check_colon_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt to -Xlog:gc

Thank you for submitting the pull request to the Apache Ignite.

In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:

The Contribution Checklist

There is a single JIRA ticket related to the pull request.
The web-link to the pull request is attached to the JIRA ticket.
The JIRA ticket has the Patch Available state.
The pull request body describes changes that have been made.
The description explains WHAT and WHY was made instead of HOW.
The pull request title is treated as the final commit message.
The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
A reviewer has been mentioned through the JIRA comments
(see the Maintainers list)
The pull request has been checked by the Teamcity Bot and
the green visa attached to the JIRA ticket (see TC.Bot: Check PR)

Notes

If you need any help, please email [email protected] or ask anу advice on http://asf.slack.com #ignite channel.

- changed default settings for gc log (-Xloggc ->-Xlog:gc) - added new logs for safepoints - added new variable to disable safepoints log - removed UnlockCommercialFeatures because it was used only for jfr, but no longer needed for openjdk11 - removed check_boolean_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt, because there is no longer an option in settings - changed option in check_colon_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt to -Xlog:gc

skorotkov · 2024-09-17T04:10:29Z

modules/ducktests/tests/checks/utils/check_ignite_spec.py

@@ -79,19 +78,10 @@ def check_default_jvm_options__are_not_used__if_merge_with_default_is_false(serv
    assert len(spec.jvm_opts) == 0


-def check_boolean_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt(service):


Even if the UnlockCommercialFeatures option is not used for JVM 11+ anymore it's not a reason to drop the test for the boolean options support. Just choose another boolean option. The -XX:+FlightRecorder would be ok I think.

Returned the test, tweaked it to check FlightRecorder in options

skorotkov · 2024-09-17T04:14:30Z

modules/ducktests/tests/ignitetest/services/utils/ignite_spec.py

@@ -102,6 +102,10 @@ def __get_default_jvm_opts(self):
                                               oom_path=os.path.join(self.service.log_dir, "out_of_mem.hprof"),
                                               vm_error_path=os.path.join(self.service.log_dir, "hs_err_pid%p.log"))

+        if not self.service.context.globals.get(SAFEPOINT_LOGS_DISABLED, False):
+            default_jvm_opts = merge_jvm_settings(
+                default_jvm_opts, ["-Xlog:safepoint:" + os.path.join(self.service.log_dir, "safepoint.log")])


Please use more readable settings for log file like:

-Xlog:safepoint*=debug:file=/mnt/service/logs/safepoint.log:time,uptime,level,tags

skorotkov · 2024-09-17T04:15:34Z

modules/ducktests/tests/ignitetest/services/utils/jvm_utils.py

    """
    Provides settings string for JVM process.
    param opts: JVM options to merge. Adds new or rewrites default values. Can be list or string.
    """
    gc_dump = ""
    if gc_dump_path:
-        gc_dump = "-verbose:gc -Xloggc:" + gc_dump_path
+        gc_dump = "-Xlog:gc:" + gc_dump_path


Please use more readable settings for log file like

-Xlog:gc*=debug,gc+stats*=debug,gc+ergo*=debug:/mnt/service/logs/gc.log:uptime,time,level,tags

skorotkov · 2024-09-17T04:15:50Z

modules/ducktests/tests/ignitetest/services/utils/jvm_utils.py

@@ -31,14 +31,14 @@


 def create_jvm_settings(heap_size=DEFAULT_HEAP, gc_settings=JVM_PARAMS_GC_G1, generic_params=JVM_PARAMS_GENERIC,
-                        gc_dump_path=None, oom_path=None, vm_error_path=None):
+                        gc_dump_path=None, oom_path=None, vm_error_path=None,):


Why this change?

By mistake, removed it

skorotkov · 2024-09-17T04:23:15Z

modules/ducktests/tests/ignitetest/utils/ignite_test.py

@@ -27,6 +27,7 @@
 # globals:
 JFR_ENABLED = "jfr_enabled"
 IGNITE_TEST_CONTEXT_CLASS_KEY_NAME = "IgniteTestContext"
+SAFEPOINT_LOGS_DISABLED = "SafePointLogsDisabled"


Please use the snake case for global variable name (in other words follow the existing approach of already existing variables: jfr_enabled, ignite_versions, jmx_remote etc.)

skorotkov · 2024-09-17T04:26:51Z

modules/ducktests/tests/ignitetest/utils/ignite_test.py

@@ -27,6 +27,7 @@
 # globals:
 JFR_ENABLED = "jfr_enabled"
 IGNITE_TEST_CONTEXT_CLASS_KEY_NAME = "IgniteTestContext"
+SAFEPOINT_LOGS_DISABLED = "SafePointLogsDisabled"


Please also change it *_enabled (with True by default). In all other variables we have enabled but not disabled.

Say "safepoint_log_enabled".

just for record

After all it was decided to have it False by default to not increase total logs size in results for tests which do not need it

- fixed PR threads

- Added special run options to the Readme - Fixed few small issues in Readme

- Fixed check_colon_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt check

- Changed readme. - safepoint log now disbled by default.

skorotkov · 2024-09-23T09:08:49Z

modules/ducktests/README.md

+      }
+    }
+```
+Options `key_store_jks` and `trust_store_jks` is paths.


... are paths

skorotkov · 2024-09-23T09:09:23Z

modules/ducktests/README.md

+    }
+```
+Options `key_store_jks` and `trust_store_jks` is paths.
+If you start it with `/` it will be used as an absolute path, otherwise as a relative path.


Relative to which directory ?

- Changed readme.

skorotkov · 2024-09-23T11:53:20Z

modules/ducktests/tests/checks/utils/check_ignite_spec.py

-    assert spec.jvm_opts.index("-XX:-UnlockCommercialFeatures") >\
-           spec.jvm_opts.index("-XX:+UnlockCommercialFeatures")
+    spec = IgniteApplicationSpec(service, jvm_opts="-XX:-FlightRecorder")
+    assert "-XX:-FlightRecorder" in spec.jvm_opts


We have here 2 identical asserts.

I think one of them should be replaced with the
assert "-XX:+FlightRecorder" in spec.jvm_opts

Otherwise it doesn't make sense.

- Changed readme. - Changed test check_boolean_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt

sonarcloud · 2024-09-23T18:12:48Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

skorotkov reviewed Sep 17, 2024

View reviewed changes

IGNITE-23196 [ducktests] Change JVM options to JDK11 ones

6ed69ec

- fixed PR threads

okreda1 force-pushed the IGNITE-23196_Change_JVM_options_to_JDK11_ones branch from d1e8ac9 to 9df0689 Compare September 20, 2024 07:24

[ducktests] Change JVM options to JDK11 ones

8984375

- Added special run options to the Readme - Fixed few small issues in Readme

okreda1 force-pushed the IGNITE-23196_Change_JVM_options_to_JDK11_ones branch from 9df0689 to 8984375 Compare September 20, 2024 11:18

okreda1 added 2 commits September 20, 2024 15:14

[ducktests] Change JVM options to JDK11 ones

8420073

- Fixed check_colon_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt check

[ducktests] Change JVM options to JDK11 ones

76b2db9

- Changed readme. - safepoint log now disbled by default.

skorotkov reviewed Sep 23, 2024

View reviewed changes

[ducktests] Change JVM options to JDK11 ones

32a56aa

- Changed readme.

skorotkov reviewed Sep 23, 2024

View reviewed changes

[ducktests] Change JVM options to JDK11 ones

d647c0a

- Changed readme. - Changed test check_boolean_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt

okreda1 force-pushed the IGNITE-23196_Change_JVM_options_to_JDK11_ones branch from 88d3c45 to d647c0a Compare September 23, 2024 12:02

skorotkov approved these changes Sep 23, 2024

View reviewed changes

ivandasch merged commit a86e0c8 into apache:master Sep 23, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-23196 [ducktests] Change JVM options to JDK11 ones #11536

IGNITE-23196 [ducktests] Change JVM options to JDK11 ones #11536

okreda1 commented Sep 16, 2024 •

edited

Loading

skorotkov Sep 17, 2024 •

edited

Loading

okreda1 Sep 20, 2024

skorotkov Sep 17, 2024

okreda1 Sep 20, 2024

skorotkov Sep 17, 2024

okreda1 Sep 20, 2024

skorotkov Sep 17, 2024

okreda1 Sep 20, 2024

skorotkov Sep 17, 2024

okreda1 Sep 20, 2024

skorotkov Sep 17, 2024

okreda1 Sep 20, 2024

skorotkov Sep 23, 2024

skorotkov Sep 23, 2024

okreda1 Sep 23, 2024

skorotkov Sep 23, 2024

okreda1 Sep 23, 2024

skorotkov Sep 23, 2024

okreda1 Sep 23, 2024

sonarcloud bot commented Sep 23, 2024

		@@ -79,19 +78,10 @@ def check_default_jvm_options__are_not_used__if_merge_with_default_is_false(serv
		assert len(spec.jvm_opts) == 0


		def check_boolean_options__go_after_default_ones_and_overwrite_them__if_passed_via_jvm_opt(service):

IGNITE-23196 [ducktests] Change JVM options to JDK11 ones #11536

IGNITE-23196 [ducktests] Change JVM options to JDK11 ones #11536

Conversation

okreda1 commented Sep 16, 2024 • edited Loading

The Contribution Checklist

Notes

skorotkov Sep 17, 2024 • 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

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

sonarcloud bot commented Sep 23, 2024

Quality Gate passed

okreda1 commented Sep 16, 2024 •

edited

Loading

skorotkov Sep 17, 2024 •

edited

Loading