EgorBaranovEnjoysTyping commented on code in PR #13413:
URL: https://github.com/apache/ignite/pull/13413#discussion_r3822540207
##########
modules/ducktests/tests/ignitetest/services/utils/ignite_spec.py:
##########
@@ -342,17 +403,11 @@ class IgniteApplicationSpec(IgniteSpec):
"""
Spec to run ignite application
"""
- def __init__(self, service, jvm_opts=None, merge_with_default=True):
- super().__init__(
- service,
- merge_jvm_settings(self.__get_default_jvm_opts() if
merge_with_default else [],
- jvm_opts if jvm_opts else []),
- merge_with_default)
-
- def __get_default_jvm_opts(self):
+ def _service_defaults(self):
return [
"-DIGNITE_NO_SHUTDOWN_HOOK=true", # allows performing operations
on app termination.
"-Xmx1G",
+ "-Xms1G", # kept in step with -Xmx: _remove_duplicates drops the
base -Xmx but keeps the base -Xms.
Review Comment:
would not work without always pre touch feature
##########
modules/ducktests/tests/ignitetest/services/utils/ignite_spec.py:
##########
@@ -91,14 +110,52 @@ def __init__(self, service, jvm_opts=None,
merge_with_default=True):
default options will be applied.
"""
self.service = service
- self.jvm_opts = merge_jvm_settings(self.__get_default_jvm_opts() if
merge_with_default else [],
- jvm_opts if jvm_opts else [])
+
+ # The caller's delta is kept alongside the merged result so that
another service can inherit the
+ # tuning a test applied here without inheriting this service's
role-dependent options too.
+ # See rebuild_as() -- copying the resolved list across services is
what broke the CDC path.
Review Comment:
I don't think it's related to feature
##########
modules/ducktests/tests/ignitetest/services/utils/jvm_utils.py:
##########
@@ -17,25 +17,85 @@
This module contains JVM utilities.
"""
+import re
+
from ignitetest.services.utils.decorators import memoize
DEFAULT_HEAP = "768M"
-JVM_PARAMS_GC_G1 = "-XX:+UseG1GC -XX:MaxGCPauseMillis=100 " \
- "-XX:ConcGCThreads=$(((`nproc`/3)>1?(`nproc`/3):1)) " \
- "-XX:ParallelGCThreads=$(((`nproc`*3/4)>1?(`nproc`*3/4):1))
"
+GC_G1 = "G1"
+GC_PARALLEL = "PARALLEL"
+GC_SERIAL = "SERIAL"
+GC_Z = "ZGC"
+GC_SHENANDOAH = "SHENANDOAH"
+
+DEFAULT_GC = GC_G1
+
+# NOTE: these strings are interpolated into a shell command that is evaluated
on the remote
+# node (see IgniteSpec._jvm_opts and IgniteNodeSpec.command), which is what
makes the `nproc`
+# substitutions work. Consequently NO option here may contain spaces or quotes.
+_NPROC_THIRD = "$(((`nproc`/3)>1?(`nproc`/3):1))"
+_NPROC_THREE_QUARTERS = "$(((`nproc`*3/4)>1?(`nproc`*3/4):1))"
+
+# Garbage collector profiles. A profile is a mutually exclusive group: it both
selects the collector
+# and carries the tuning flags that are meaningful for it. Never mix flags
across profiles.
+GC_PROFILES = {
Review Comment:
nit: It looks like it's better to put it in separate file to keep this
module cleaner..
##########
modules/ducktests/tests/ignitetest/services/utils/jvm_utils.py:
##########
@@ -17,25 +17,85 @@
This module contains JVM utilities.
"""
+import re
+
from ignitetest.services.utils.decorators import memoize
DEFAULT_HEAP = "768M"
-JVM_PARAMS_GC_G1 = "-XX:+UseG1GC -XX:MaxGCPauseMillis=100 " \
- "-XX:ConcGCThreads=$(((`nproc`/3)>1?(`nproc`/3):1)) " \
- "-XX:ParallelGCThreads=$(((`nproc`*3/4)>1?(`nproc`*3/4):1))
"
+GC_G1 = "G1"
+GC_PARALLEL = "PARALLEL"
+GC_SERIAL = "SERIAL"
+GC_Z = "ZGC"
+GC_SHENANDOAH = "SHENANDOAH"
+
+DEFAULT_GC = GC_G1
+
+# NOTE: these strings are interpolated into a shell command that is evaluated
on the remote
+# node (see IgniteSpec._jvm_opts and IgniteNodeSpec.command), which is what
makes the `nproc`
+# substitutions work. Consequently NO option here may contain spaces or quotes.
+_NPROC_THIRD = "$(((`nproc`/3)>1?(`nproc`/3):1))"
+_NPROC_THREE_QUARTERS = "$(((`nproc`*3/4)>1?(`nproc`*3/4):1))"
+
+# Garbage collector profiles. A profile is a mutually exclusive group: it both
selects the collector
+# and carries the tuning flags that are meaningful for it. Never mix flags
across profiles.
+GC_PROFILES = {
+ GC_G1: [
+ "-XX:+UseG1GC",
+ "-XX:MaxGCPauseMillis=100",
+ f"-XX:ConcGCThreads={_NPROC_THIRD}",
+ f"-XX:ParallelGCThreads={_NPROC_THREE_QUARTERS}",
+ "-XX:+UseStringDeduplication", # G1-only until JDK 18, hence part of
the profile
+ ],
+ GC_PARALLEL: [
+ "-XX:+UseParallelGC",
+ f"-XX:ParallelGCThreads={_NPROC_THREE_QUARTERS}",
+ # deliberately NO MaxGCPauseMillis: it flips ParallelGC into adaptive
pause-goal sizing
+ ],
+ GC_SERIAL: [
+ "-XX:+UseSerialGC",
+ ],
+ GC_Z: [
+ "-XX:+UseZGC", # product feature since JDK 15, no unlock flag needed
+ f"-XX:ConcGCThreads={_NPROC_THIRD}",
+ f"-XX:ParallelGCThreads={_NPROC_THREE_QUARTERS}",
+ ],
+ GC_SHENANDOAH: [
+ "-XX:+UseShenandoahGC", # product feature since JDK 15; OpenJDK only,
not Oracle JDK
+ f"-XX:ConcGCThreads={_NPROC_THIRD}",
+ f"-XX:ParallelGCThreads={_NPROC_THREE_QUARTERS}",
+ ],
+}
JVM_PARAMS_GENERIC = "-server -XX:+DisableExplicitGC -XX:+AlwaysPreTouch " \
"-XX:+ParallelRefProcEnabled -XX:+DoEscapeAnalysis " \
- "-XX:+OptimizeStringConcat -XX:+UseStringDeduplication"
+ "-XX:+OptimizeStringConcat"
+
+# Matches a collector selector like -XX:+UseZGC. Deliberately narrow: it must
not match
+# -XX:+DisableExplicitGC or -XX:+UseStringDeduplication.
+_GC_SELECTOR_PATTERN = re.compile(r"^-XX:([+-])(Use\w+GC)$")
+
+
+class MultipleGcSelectedError(Exception):
+ """
+ Raised when JVM options end up selecting more than one garbage collector.
+ """
-def create_jvm_settings(heap_size=DEFAULT_HEAP, gc_settings=JVM_PARAMS_GC_G1,
generic_params=JVM_PARAMS_GENERIC,
+def create_jvm_settings(heap_size=DEFAULT_HEAP, gc_settings=None,
generic_params=JVM_PARAMS_GENERIC,
gc_dump_path=None, oom_path=None, vm_error_path=None):
"""
Provides settings string for JVM process.
- param opts: JVM options to merge. Adds new or rewrites default values. Can
be list or string.
+ :param heap_size: value for both -Xmx and -Xms.
+ :param gc_settings: garbage collector options, see GC_PROFILES. Can be
list or string.
+ Defaults to the DEFAULT_GC profile.
+ :param generic_params: collector-independent options. Can be list or
string.
"""
+ gc_settings = GC_PROFILES[DEFAULT_GC] if gc_settings is None else
gc_settings
+
+ if isinstance(gc_settings, str):
Review Comment:
otherwise it will fail at line 111?
##########
modules/ducktests/tests/ignitetest/services/utils/gc_params.py:
##########
@@ -0,0 +1,88 @@
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License
+
+"""
+This module resolves the garbage collector to use from Globals.
+
+GC selection is mutually-exclusive group replacement: a collector and its
tuning flags travel together
+(see GC_PROFILES in jvm_utils). It therefore has to be chosen *before* the
default option list is
+assembled -- patching it afterwards via jvm_opts leaves two selectors in the
command line, because
+merge_jvm_settings overwrites per option, not per group.
+
+This is the single resolution point for the 'gc' global. Keep it that way.
+"""
+
+from ignitetest.services.utils.jvm_utils import DEFAULT_GC, GC_PROFILES
+
+GC_KEY_NAME = "gc"
+
+SERVER_ROLE = "server"
+CLIENT_ROLE = "client"
+
+
+def is_gc_configured(_globals: dict):
+ """
+ :param _globals: Globals parameters
+ :return: True if the run explicitly selects a garbage collector.
+ """
+ return GC_KEY_NAME in (_globals or {})
+
+
+def resolve_gc_settings(_globals: dict, role: str):
+ """
+ Gets garbage collector options from Globals. Three shapes are accepted:
+
+ {"gc": "ZGC"} -- both roles
+ {"gc": {"server": "ZGC"}} -- servers only,
clients keep the default
+ {"gc": {"server": "ZGC", "client": "SERIAL"}} -- per role
+ {"gc": {"server": ["-XX:+UseZGC", "-XX:SoftMaxHeapSize=2G"]}} -- raw
options, escape hatch
+
+ Profile names are case-insensitive. A missing role, or a missing 'gc' key,
yields the DEFAULT_GC
+ profile. A list value is used verbatim and bypasses profile validation --
that is the point of it.
+
+ :param _globals: Globals parameters
+ :param role: SERVER_ROLE or CLIENT_ROLE
+ :return: list of JVM options selecting and tuning the collector
+ """
+ configured = (_globals or {}).get(GC_KEY_NAME)
+
+ if configured is None:
+ return _profile(DEFAULT_GC)
+
+ if isinstance(configured, dict):
+ configured = configured.get(role)
+
+ if configured is None:
+ return _profile(DEFAULT_GC)
Review Comment:
Would this case work? {"gc": "ZGC"}
##########
modules/ducktests/tests/ignitetest/services/utils/ignite_aware.py:
##########
@@ -117,10 +117,27 @@ def start_node(self, node, **kwargs):
super().start_node(node, **kwargs)
- wait_until(lambda: self.alive(node), timeout_sec=10)
+ wait_until(lambda: self.alive(node), timeout_sec=10,
+ err_msg=lambda: self.__jvm_startup_failure_msg(node))
ignite_jmx_mixin(node, self)
+ def __jvm_startup_failure_msg(self, node):
+ """
+ A JVM that rejects an option (an unknown collector, a bad heap value,
two collectors selected)
+ dies before it logs anything Ignite-shaped, so without the console
tail this is a bare timeout.
+ """
+ console_log = os.path.join(self.log_dir, "console.log")
+
+ try:
+ tail = "".join(node.account.ssh_capture(f"tail -n 30
{console_log}", allow_fail=True))
+ except Exception as err: # pylint: disable=broad-except
+ # Never let diagnostics mask the timeout they are diagnosing.
+ tail = f"<unable to read {console_log}: {err}>"
Review Comment:
Could u pls add some info about process where it fails. Without stacktrace
and code it's not clear what's happened
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
