timoninmaxim commented on a change in pull request #8098:
URL: https://github.com/apache/ignite/pull/8098#discussion_r462397481
##########
File path: modules/ducktests/tests/ignitetest/services/utils/ignite_path.py
##########
@@ -13,44 +13,55 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+"""
+This module contains ignite path resolve utilities.
+"""
+
import os
from ignitetest.tests.utils.version import get_version, IgniteVersion
-"""
-This module provides Ignite path methods
-"""
-
class IgnitePath:
- SCRATCH_ROOT = "/mnt"
- IGNITE_INSTALL_ROOT = "/opt"
-
"""Path resolver for Ignite system tests which assumes the following
layout:
- /opt/ignite-dev # Current version of Ignite under test
- /opt/ignite-2.7.6 # Example of an older version of Ignite
installed from tarball
- /opt/ignite-<version> # Other previous versions of Ignite
- ...
- """
+ /opt/ignite-dev # Current version of Ignite under test
+ /opt/ignite-2.7.6 # Example of an older version of Ignite
installed from tarball
+ /opt/ignite-<version> # Other previous versions of Ignite
+ ...
+ """
+ SCRATCH_ROOT = "/mnt"
+ IGNITE_INSTALL_ROOT = "/opt"
def __init__(self, context):
self.project = context.globals.get("project", "ignite")
def home(self, node_or_version, project=None):
- version = self._version(node_or_version)
+ """
+ :param node_or_version: Ignite service node or IgniteVersion instance.
+ :param project: Project name.
+ :return: Home directory.
+ """
+ version = self.__version__(node_or_version)
home_dir = project or self.project
if version is not None:
home_dir += "-%s" % str(version)
return os.path.join(IgnitePath.IGNITE_INSTALL_ROOT, home_dir)
def script(self, script_name, node_or_version, project=None):
- version = self._version(node_or_version)
+ """
+ :param script_name: Script name.
+ :param node_or_version: Ignite service node or IgniteVersion instance.
+ :param project: Project name.
+ :return: Full path to script.
+ """
+ version = self.__version__(node_or_version)
return os.path.join(self.home(version, project=project), "bin",
script_name)
- def _version(self, node_or_version):
+ @staticmethod
+ def __version__(node_or_version):
Review comment:
Methods with double underscore on both sides have [special
meaning](https://diveintopython3.net/special-method-names.html). Such methods
are mixins and doesn't suppose to be just private methods. Private methods have
another naming: __version, without trailing underscores.
##########
File path: modules/ducktests/tests/ignitetest/services/spark.py
##########
@@ -103,28 +109,40 @@ def clean_node(self, node):
node.account.ssh("sudo rm -rf -- %s" %
SparkService.SPARK_PERSISTENT_ROOT, allow_fail=False)
def pids(self, node):
- """Return process ids associated with running processes on the given
node."""
try:
cmd = "jcmd | grep -e %s | awk '{print $1}'" %
self.java_class_name(node)
- pid_arr = [pid for pid in node.account.ssh_capture(cmd,
allow_fail=True, callback=int)]
- return pid_arr
- except (RemoteCommandError, ValueError) as e:
+ return list(node.account.ssh_capture(cmd, allow_fail=True,
callback=int))
+ except (RemoteCommandError, ValueError):
return []
def java_class_name(self, node):
+ """
+ :param node: Spark node.
+ :return: Class name depending on node type (master or slave).
+ """
if node == self.nodes[0]:
return "org.apache.spark.deploy.master.Master"
- else:
- return "org.apache.spark.deploy.worker.Worker"
- def master_log_path(self, node):
+ return "org.apache.spark.deploy.worker.Worker"
+
+ @staticmethod
Review comment:
Do we really need static methods in our code? I don't see any case in
our code where usage of them is required. It just add more sugar without need.
Also some code styles consider them evil, e.g. [Google
Guide](https://google.github.io/styleguide/pyguide.html#217-function-and-method-decorators).
##########
File path: modules/ducktests/tests/ignitetest/services/utils/ignite_path.py
##########
@@ -13,44 +13,55 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+"""
+This module contains ignite path resolve utilities.
+"""
+
import os
from ignitetest.tests.utils.version import get_version, IgniteVersion
-"""
-This module provides Ignite path methods
-"""
-
class IgnitePath:
- SCRATCH_ROOT = "/mnt"
- IGNITE_INSTALL_ROOT = "/opt"
-
"""Path resolver for Ignite system tests which assumes the following
layout:
- /opt/ignite-dev # Current version of Ignite under test
- /opt/ignite-2.7.6 # Example of an older version of Ignite
installed from tarball
- /opt/ignite-<version> # Other previous versions of Ignite
- ...
- """
+ /opt/ignite-dev # Current version of Ignite under test
+ /opt/ignite-2.7.6 # Example of an older version of Ignite
installed from tarball
+ /opt/ignite-<version> # Other previous versions of Ignite
+ ...
+ """
+ SCRATCH_ROOT = "/mnt"
+ IGNITE_INSTALL_ROOT = "/opt"
def __init__(self, context):
self.project = context.globals.get("project", "ignite")
def home(self, node_or_version, project=None):
- version = self._version(node_or_version)
+ """
+ :param node_or_version: Ignite service node or IgniteVersion instance.
+ :param project: Project name.
+ :return: Home directory.
+ """
+ version = self.__version__(node_or_version)
home_dir = project or self.project
if version is not None:
home_dir += "-%s" % str(version)
return os.path.join(IgnitePath.IGNITE_INSTALL_ROOT, home_dir)
def script(self, script_name, node_or_version, project=None):
- version = self._version(node_or_version)
+ """
+ :param script_name: Script name.
+ :param node_or_version: Ignite service node or IgniteVersion instance.
+ :param project: Project name.
+ :return: Full path to script.
+ """
+ version = self.__version__(node_or_version)
return os.path.join(self.home(version, project=project), "bin",
script_name)
- def _version(self, node_or_version):
+ @staticmethod
Review comment:
there is no need for staticmethod decorator as the method is never used
beyond this class
##########
File path: modules/ducktests/tests/ignitetest/services/spark.py
##########
@@ -103,28 +109,40 @@ def clean_node(self, node):
node.account.ssh("sudo rm -rf -- %s" %
SparkService.SPARK_PERSISTENT_ROOT, allow_fail=False)
def pids(self, node):
- """Return process ids associated with running processes on the given
node."""
try:
cmd = "jcmd | grep -e %s | awk '{print $1}'" %
self.java_class_name(node)
- pid_arr = [pid for pid in node.account.ssh_capture(cmd,
allow_fail=True, callback=int)]
- return pid_arr
- except (RemoteCommandError, ValueError) as e:
+ return list(node.account.ssh_capture(cmd, allow_fail=True,
callback=int))
+ except (RemoteCommandError, ValueError):
return []
def java_class_name(self, node):
+ """
+ :param node: Spark node.
+ :return: Class name depending on node type (master or slave).
+ """
if node == self.nodes[0]:
return "org.apache.spark.deploy.master.Master"
- else:
- return "org.apache.spark.deploy.worker.Worker"
- def master_log_path(self, node):
+ return "org.apache.spark.deploy.worker.Worker"
+
+ @staticmethod
Review comment:
Do we really need static methods in our code? I don't see any case in
our code where usage of them is required. It just adds more sugar without need.
Also some code styles consider them evil, e.g. [Google
Guide](https://google.github.io/styleguide/pyguide.html#217-function-and-method-decorators).
----------------------------------------------------------------
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]
