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:54:06 UTC
[GitHub] [ignite] ivandasch opened a new pull request #8098: Ducktape codestyle
ivandasch opened a new pull request #8098:
URL: https://github.com/apache/ignite/pull/8098
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](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
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
[GitHub] [ignite] ivandasch commented on a change in pull request #8098: Ducktape codestyle
Posted by GitBox <gi...@apache.org>.
ivandasch commented on a change in pull request #8098:
URL: https://github.com/apache/ignite/pull/8098#discussion_r462762238
##########
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:
Despite the fact that in mentioned above codestyle staticmethods are not welcome in cod, linters generate warnings about unused `self`. And usage of `@staticmethod` is a preferrable way to mitigate this case. This is so common in my practice and widely accepted in a python's community.
##########
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:
Despite the fact that in mentioned above codestyle staticmethods are not welcome in code, linters generate warnings about unused `self`. And usage of `@staticmethod` is a preferrable way to mitigate this case. This is so common in my practice and widely accepted in a python's community.
----------------------------------------------------------------
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
[GitHub] [ignite] ivandasch commented on a change in pull request #8098: Ducktape codestyle
Posted by GitBox <gi...@apache.org>.
ivandasch commented on a change in pull request #8098:
URL: https://github.com/apache/ignite/pull/8098#discussion_r462420728
##########
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:
Both pycharm and pylint generate warnings on it.
##########
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:
Yep, you are right
##########
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:
Yep, you are right
----------------------------------------------------------------
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
[GitHub] [ignite] anton-vinogradov merged pull request #8098: Ducktape codestyle
Posted by GitBox <gi...@apache.org>.
anton-vinogradov merged pull request #8098:
URL: https://github.com/apache/ignite/pull/8098
----------------------------------------------------------------
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
[GitHub] [ignite] timoninmaxim commented on a change in pull request #8098: Ducktape codestyle
Posted by GitBox <gi...@apache.org>.
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