anton-vinogradov commented on a change in pull request #8660: URL: https://github.com/apache/ignite/pull/8660#discussion_r562484083
########## File path: modules/ducktests/tests/checks/utils/check_tools.py ########## @@ -0,0 +1,112 @@ +# 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. + +""" +Checks JVM settings. +""" + +from ignitetest.services.utils.jvm_utils import create_jvm_settings, merge_jvm_settings, DEFAULT_HEAP + + +class CheckJVMSettings: Review comment: Could we rename the file (check-tools.py) to make clear it's about JVM settings? ########## File path: modules/ducktests/tests/ignitetest/services/utils/jvm_utils.py ########## @@ -0,0 +1,124 @@ +# 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 contains JVM utilities. +""" +DEFAULT_HEAP = "768M" + +JVM_PARAMS_GC_CMS = "-XX:+UseConcMarkSweepGC -XX:+UseParNewGC -XX:+CMSIncrementalMode " \ + "-XX:ConcGCThreads=$(((`nproc`/4)>1?(`nproc`/4):1)) " \ + "-XX:ParallelGCThreads=$(((`nproc`/2)>1?(`nproc`/2):1)) " \ + "-XX:CMSInitiatingOccupancyFraction=70 -XX:+UseCMSInitiatingOccupancyOnly " \ + "-XX:+CMSParallelRemarkEnabled -XX:+CMSClassUnloadingEnabled" + +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)) " + +JVM_PARAMS_GENERIC = "-server -XX:+DisableExplicitGC -XX:+AggressiveOpts -XX:+AlwaysPreTouch " \ + "-XX:+ParallelRefProcEnabled -XX:+DoEscapeAnalysis " \ + "-XX:+OptimizeStringConcat -XX:+UseStringDeduplication" + + +def create_jvm_settings(heap_size=DEFAULT_HEAP, gc_settings=JVM_PARAMS_GC_CMS, generic_params=JVM_PARAMS_GENERIC, + gc_dump_path=None, oom_path=None, **kwargs): + """ + Provides settings string for JVM process. + param as_list: Represent JVM params as list. + param as_map: Represent JVM params as dict. + param opts: JVM options to merge. Adds new or rewrites default values. Can be dict, list or string. + """ + gc_dump = "" + if gc_dump_path: + gc_dump = "-XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=5 -XX:GCLogFileSize=32M -XX:+PrintGCDateStamps " \ + "-verbose:gc -XX:+PrintGCDetails -Xloggc:" + gc_dump_path + + out_of_mem_dump = "" + if oom_path: + out_of_mem_dump = "-XX:+HeapDumpOnOutOfMemoryError -XX:HeapDumpPath=" + oom_path + + as_string = f"-Xmx{heap_size} -Xms{heap_size} {gc_settings} {gc_dump} {out_of_mem_dump} {generic_params}".strip() + + to_merge = kwargs.get("opts") + + if to_merge or kwargs.get("as_map"): + return merge_jvm_settings(as_string, to_merge if to_merge else {}, **kwargs) + + if kwargs.get("as_list"): + return as_string.split() + + return as_string Review comment: Seems, everywhere (for create_jvm_settings and merge_jvm_settings) we have the result returned as a list. Do we really need to have this returned as a map and string? My vote is, as always, for minimalism and simplification instead of premature complexion. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
