You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/07/29 17:52:33 UTC
[GitHub] [ignite] timoninmaxim commented on a change in pull request #8098: Ducktape codestyle
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:
users@infra.apache.org