You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2021/09/21 10:29:16 UTC
[GitHub] [mesos] andreaspeters opened a new pull request #409: Remove tasks limit and condition. Add global functions.
andreaspeters opened a new pull request #409:
URL: https://github.com/apache/mesos/pull/409
Like I promised in PR#405 (https://github.com/apache/mesos/pull/405), this is one of the three PR's I will open. This PR will change:
- Remove unneeded conditions parameter to "filter" the json call.
- Remove the limitation of "task list".
- Add global function to get the host address of a framework (for future use in plugins)
- Add global function to get all frameworks connected to mesos (for future use in plugins)
@cf-natali I will open the other two PR's when this one is reviewed. :-)
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r716039046
##########
File path: src/python/cli_new/lib/cli/mesos.py
##########
@@ -68,6 +63,35 @@ def get_agents(master, config):
"""
endpoint = "slaves"
key = "slaves"
+ return get_key_endpoint(key, endpoint, master, config)
+
+def get_framework_address(framework_id, master, config):
+ """
+ Given a master and an framework id, return the framework address
+ by checking the /master/frameworks endpoint of the master.
+ """
+ frameworks = get_frameworks(master, config)
+
+ for framework in frameworks:
+ if framework["id"] == framework_id:
+ return framework["webui_url"]
+ # pylint: disable=line-too-long
+ raise CLIException("Unable to find framework '{id}'".format(id=framework_id))
Review comment:
Maybe better to indent on a new line than disable the pylint warning?
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r716425552
##########
File path: src/python/cli_new/lib/cli/tests/base.py
##########
@@ -272,41 +274,33 @@ def launch(self, timeout=TIMEOUT):
super(Agent, self).launch()
Agent.count += 1
- try:
- # pylint: disable=missing-docstring
- def single_slave(data):
- return len(data["slaves"]) == 1
-
- http.get_json(self.flags["master"], "slaves", single_slave, timeout)
- except Exception as exception:
+ data = http.get_json(self.flags["master"], "slaves", self.config)
+ if len(data["slaves"]) == 1:
Review comment:
yes, it is the same. But since the get_json function does not have the condition, I have to change it a little.Before, the function single_slave() was a parameter (condition) for get_json(). And in get_json there was a call for condition.
No matter, I think the whole test never really worked before. I had a look inside. The mesos-cli test even check if it's possible to start a task. But, the mesos-cli cannot start a task. :man_shrugging: So, I think the whole test should be rewritten. But thats nothing I will do at the moment.
What I'm doing is, I'm using mesos-cli by self at least some weeks long before my PR.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r713338428
##########
File path: src/python/cli_new/lib/cli/http.py
##########
@@ -64,13 +64,11 @@ def read_endpoint(addr, endpoint, config, query=None):
.format(url=url, error=str(exception)))
-def get_json(addr, endpoint, config, condition=None, query=None):
+def get_json(addr, endpoint, config, query=None):
Review comment:
So interestingly the only call site which seems to pass 4 arguments to `get_json` is this: https://github.com/apache/mesos/blob/c66aaf47768aa0e5b66ee9d830f634f4dd682c43/src/python/cli_new/lib/cli/tests/base.py#L280
However it's apparently passing a `timeout` argument, which doesn't seem to be accepted here.
Looking at `git blame`, I think you might have broken it here: https://github.com/apache/mesos/commit/c66aaf47768aa0e5b66ee9d830f634f4dd682c43#diff-1b933ba1cadd6e125668ca2646e6376b740919e25e905142108f9cc324bafb4a
As you can see you removed the `timeout` parameter from `get_json` but the test still uses it.
So I have two questions:
- have you run the tests, or do you know how to run them?
- we should probably fix the test as well
##########
File path: src/python/cli_new/lib/cli/mesos.py
##########
@@ -68,6 +63,37 @@ def get_agents(master, config):
"""
endpoint = "slaves"
key = "slaves"
+ return get_key_endpoint(key, endpoint, master, config)
+
+def get_framework_address(framework_id, master, config):
+ """
+ Given a master and an framework id, return the framework address
+ by checking the /master/frameworks endpoint of the master.
+ """
+ frameworks = get_frameworks(master, config)
+
+ for framework in frameworks:
+ if framework["id"] == framework_id:
+ return framework["webui_url"]
+ # pylint: disable=line-too-long
+ raise CLIException("Unable to find framework '{id}'".format(id=framework_id))
+
+
+def get_frameworks(master, config):
+ """
+ Get the frameworks in a Mesos cluster.
+ """
+
+ endpoint = "master/frameworks/"
+ key = "frameworks"
+ return get_key_endpoint(key, endpoint, master, config)
+
+
+def get_key_endpoint(key, endpoint, master, config):
+ """
+ Get the json key of the given endpoint
+ """
+
Review comment:
Nit: no need for newline.
##########
File path: src/python/cli_new/lib/cli/mesos.py
##########
@@ -68,6 +63,37 @@ def get_agents(master, config):
"""
endpoint = "slaves"
key = "slaves"
+ return get_key_endpoint(key, endpoint, master, config)
+
+def get_framework_address(framework_id, master, config):
+ """
+ Given a master and an framework id, return the framework address
+ by checking the /master/frameworks endpoint of the master.
+ """
+ frameworks = get_frameworks(master, config)
+
+ for framework in frameworks:
+ if framework["id"] == framework_id:
+ return framework["webui_url"]
+ # pylint: disable=line-too-long
+ raise CLIException("Unable to find framework '{id}'".format(id=framework_id))
+
+
+def get_frameworks(master, config):
+ """
+ Get the frameworks in a Mesos cluster.
+ """
+
Review comment:
Nit: no need for newline.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r714547218
##########
File path: src/python/cli_new/lib/cli/http.py
##########
@@ -64,13 +64,11 @@ def read_endpoint(addr, endpoint, config, query=None):
.format(url=url, error=str(exception)))
-def get_json(addr, endpoint, config, condition=None, query=None):
+def get_json(addr, endpoint, config, query=None):
Review comment:
Well, I decide to change the test functions. If u don't feel fine with it, it's ok and I will re-implement the condition parameter. :-)
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r713690519
##########
File path: src/python/cli_new/lib/cli/http.py
##########
@@ -64,13 +64,11 @@ def read_endpoint(addr, endpoint, config, query=None):
.format(url=url, error=str(exception)))
-def get_json(addr, endpoint, config, condition=None, query=None):
+def get_json(addr, endpoint, config, query=None):
Review comment:
Yes I run the test, I will have a look why it's working on my site. Because, u are right, it should fail at that point.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r716038766
##########
File path: src/python/cli_new/lib/cli/http.py
##########
@@ -64,13 +64,11 @@ def read_endpoint(addr, endpoint, config, query=None):
.format(url=url, error=str(exception)))
-def get_json(addr, endpoint, config, condition=None, query=None):
+def get_json(addr, endpoint, config, query=None):
Review comment:
Thanks, I think that's fine!
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r716039396
##########
File path: src/python/cli_new/lib/cli/tests/base.py
##########
@@ -272,41 +274,33 @@ def launch(self, timeout=TIMEOUT):
super(Agent, self).launch()
Agent.count += 1
- try:
- # pylint: disable=missing-docstring
- def single_slave(data):
- return len(data["slaves"]) == 1
-
- http.get_json(self.flags["master"], "slaves", single_slave, timeout)
- except Exception as exception:
+ data = http.get_json(self.flags["master"], "slaves", self.config)
+ if len(data["slaves"]) == 1:
Review comment:
I don't understand this change - it looks like before, the test only worked with a single slave.
Now, it looks like it will throw if there is a single slave?
```
if len(data["slaves"]) == 1:
```
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r717458647
##########
File path: src/python/cli_new/lib/cli/mesos.py
##########
@@ -68,6 +63,35 @@ def get_agents(master, config):
"""
endpoint = "slaves"
key = "slaves"
+ return get_key_endpoint(key, endpoint, master, config)
+
+def get_framework_address(framework_id, master, config):
+ """
+ Given a master and an framework id, return the framework address
+ by checking the /master/frameworks endpoint of the master.
+ """
+ frameworks = get_frameworks(master, config)
+
+ for framework in frameworks:
+ if framework["id"] == framework_id:
+ return framework["webui_url"]
+ # pylint: disable=line-too-long
+ raise CLIException("Unable to find framework '{id}'".format(id=framework_id))
Review comment:
:joy:don't know why I even add it.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r717458647
##########
File path: src/python/cli_new/lib/cli/mesos.py
##########
@@ -68,6 +63,35 @@ def get_agents(master, config):
"""
endpoint = "slaves"
key = "slaves"
+ return get_key_endpoint(key, endpoint, master, config)
+
+def get_framework_address(framework_id, master, config):
+ """
+ Given a master and an framework id, return the framework address
+ by checking the /master/frameworks endpoint of the master.
+ """
+ frameworks = get_frameworks(master, config)
+
+ for framework in frameworks:
+ if framework["id"] == framework_id:
+ return framework["webui_url"]
+ # pylint: disable=line-too-long
+ raise CLIException("Unable to find framework '{id}'".format(id=framework_id))
Review comment:
:joy:don't know why I even add it.
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali merged pull request #409: Refactored the Python CLI code.
Posted by GitBox <gi...@apache.org>.
cf-natali merged pull request #409:
URL: https://github.com/apache/mesos/pull/409
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #409: Refactored the Python CLI code.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on pull request #409:
URL: https://github.com/apache/mesos/pull/409#issuecomment-933005286
Thanks, looking forward to the other MRs!
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #409: Remove tasks limit and condition. Add global functions.
Posted by GitBox <gi...@apache.org>.
andreaspeters commented on a change in pull request #409:
URL: https://github.com/apache/mesos/pull/409#discussion_r713729146
##########
File path: src/python/cli_new/lib/cli/http.py
##########
@@ -64,13 +64,11 @@ def read_endpoint(addr, endpoint, config, query=None):
.format(url=url, error=str(exception)))
-def get_json(addr, endpoint, config, condition=None, query=None):
+def get_json(addr, endpoint, config, query=None):
Review comment:
ok, after cleanup, the test fails also on my site.
But the problem is not timeout, it's the removed condition variable. I can re-implement it. But it's only used for the test. :man_shrugging: What do u think? Shell I let it inside or remove it and change the test functions?
--
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: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org