[GitHub] [mesos] cf-natali edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
cf-natali edited a comment on pull request #391: URL: https://github.com/apache/mesos/pull/391#issuecomment-853254919 > When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like: > `0//cpu-migrations/779898/100.00/0.000/K/sec` which is different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`). Might be because `LC_ALL` is set on your machine - try running: ``` # LC_ALL=fr_FR.UTF-8 perf stat --field-separator "/" -- true ``` instead? It's also possible you don't have the `fr_FR.UTF-8` locale generated, in which case it wouldn't work. -- 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: us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r644309724
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,79 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+return self.data["master"].get("principal")
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+return self.data["master"].get("secret")
+
+def agent_ssl(self, default=False):
+"""
+Return if the agent support ssl
+"""
+if "agent" in self.data:
+agent_ssl = self.data["agent"].get("ssl", default)
+if not isinstance(agent_ssl, bool):
+raise CLIException("The 'agent->ssl' field"
+ " must be True/False")
+
+return agent_ssl
+
+return default
Review comment:
Up to you, I'm fine either way :).
--
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:
us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644309287 ## File path: src/python/cli_new/lib/cli/plugins/task/main.py ## @@ -106,12 +108,13 @@ def list(self, argv): # pylint: disable=unused-argument try: master = self.config.master() +config = self.config Review comment: OK! -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644263295 ## File path: src/python/cli_new/lib/cli/plugins/task/main.py ## @@ -106,12 +108,13 @@ def list(self, argv): # pylint: disable=unused-argument try: master = self.config.master() +config = self.config Review comment: I thinks it's easier to read. -- 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: us...@infra.apache.org
[GitHub] [mesos] andreaspeters commented on a change in pull request #383: Add mesos authentication to the mesos cli
andreaspeters commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644258228 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -137,3 +211,15 @@ def plugins(self): return self.data["plugins"] return [] + +def authentication_header(self): +""" +Return the BasicAuth authentication header +""" +if (self.agent_principal() is not None +and self.agent_secret() is not None): Review comment: The linter say it's fine like that. :man_shrugging: And my vim say, it should be like that. :joy: -- 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: us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644251140 ## File path: src/python/cli_new/lib/cli/plugins/task/main.py ## @@ -106,12 +108,13 @@ def list(self, argv): # pylint: disable=unused-argument try: master = self.config.master() +config = self.config Review comment: Why store it here and not directly pass `self.config` below? ``` try: tasks = get_tasks(master, config) ``` -- 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: us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383: URL: https://github.com/apache/mesos/pull/383#discussion_r644249567 ## File path: src/python/cli_new/lib/cli/config.py ## @@ -137,3 +211,15 @@ def plugins(self): return self.data["plugins"] return [] + +def authentication_header(self): +""" +Return the BasicAuth authentication header +""" +if (self.agent_principal() is not None +and self.agent_secret() is not None): Review comment: Nit: the indentation seems a bit strange? -- 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: us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r644249177
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,79 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+return self.data["master"].get("principal")
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+return self.data["master"].get("secret")
+
+def agent_ssl(self, default=False):
+"""
+Return if the agent support ssl
+"""
+if "agent" in self.data:
+agent_ssl = self.data["agent"].get("ssl", default)
+if not isinstance(agent_ssl, bool):
+raise CLIException("The 'agent->ssl' field"
+ " must be True/False")
+
+return agent_ssl
+
+return default
+
+def agent_ssl_verify(self, default=False):
+"""
+Return if the ssl certificate should be verified
+"""
+if "agent" in self.data:
Review comment:
Same here.
--
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:
us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #383: Add mesos authentication to the mesos cli
cf-natali commented on a change in pull request #383:
URL: https://github.com/apache/mesos/pull/383#discussion_r644249049
##
File path: src/python/cli_new/lib/cli/config.py
##
@@ -119,6 +120,79 @@ def master(self):
return master
+def principal(self):
+"""
+Return the principal in the configuration file
+"""
+return self.data["master"].get("principal")
+
+def secret(self):
+"""
+Return the secret in the configuration file
+"""
+return self.data["master"].get("secret")
+
+def agent_ssl(self, default=False):
+"""
+Return if the agent support ssl
+"""
+if "agent" in self.data:
+agent_ssl = self.data["agent"].get("ssl", default)
+if not isinstance(agent_ssl, bool):
+raise CLIException("The 'agent->ssl' field"
+ " must be True/False")
+
+return agent_ssl
+
+return default
Review comment:
So you can simplify it even further by using a default for
`self.data["agent"], like this]:
```
def agent_ssl(self, default=False):
agent_ssl = self.data.get("agent", {}).get("ssl", default)
if not isinstance(agent_ssl, bool):
raise CLIException("The 'agent->ssl' field must be True/False"
return agent_ssl
```
Personally I like it because it removes a repetition between checking that
`"agent"` is in `data` and then accessing it, and also it removes a branch, but
up to you if you find it less natural.
--
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:
us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on pull request #391: Fixed parsing of `perf` output on some locales.
cf-natali commented on pull request #391: URL: https://github.com/apache/mesos/pull/391#issuecomment-853254919 > When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like: > `0//cpu-migrations/779898/100.00/0.000/K/sec` which is different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`). Might be because `LC_ALL` is set on your machine - try running: ``` # LC_ALL=fr_FR.UTF-8 perf stat --field-separator "/" -- true ``` instead? -- 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: us...@infra.apache.org
[GitHub] [mesos] cf-natali commented on a change in pull request #391: Fixed parsing of `perf` output on some locales.
cf-natali commented on a change in pull request #391:
URL: https://github.com/apache/mesos/pull/391#discussion_r644189780
##
File path: src/linux/perf.cpp
##
@@ -125,6 +125,12 @@ class Perf : public Process
private:
void execute()
{
+// If the locale is such that `LC_NUMERIC` uses the comma ',' as decimal
+// separator, parsing won't work - because of unexpected number of fields
+// and floating points format - so make sure it's set to `C`.
+std::map env = os::environment();
+env["LC_ALL"] = "C";
Review comment:
Yep.
--
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:
us...@infra.apache.org
[GitHub] [mesos] qianzhangxa edited a comment on pull request #391: Fixed parsing of `perf` output on some locales.
qianzhangxa edited a comment on pull request #391: URL: https://github.com/apache/mesos/pull/391#issuecomment-853135728 > root@thinkpad:~# LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" > -- true > 0,31/msec/task-clock/306721/100,00/0/CPUs utilized > 0//context-switches/306721/100,00/0/K/sec > 0//cpu-migrations/306721/100,00/0/K/sec > 44//page-faults/306721/100,00/0/M/sec > 788234//cycles/311478/100,00/2/GHz > 538077//instructions/311478/100,00/0/insn per cycle > 106749//branches/311478/100,00/348/M/sec > 4556//branch-misses/311478/100,00/4/of all branches When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like: `0//cpu-migrations/779898/100.00/0.000/K/sec` which is different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`). -- 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: us...@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on pull request #391: Fixed parsing of `perf` output on some locales.
qianzhangxa commented on pull request #391: URL: https://github.com/apache/mesos/pull/391#issuecomment-853135728 > root@thinkpad:~# LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" > -- true > 0,31/msec/task-clock/306721/100,00/0/CPUs utilized > 0//context-switches/306721/100,00/0/K/sec > 0//cpu-migrations/306721/100,00/0/K/sec > 44//page-faults/306721/100,00/0/M/sec > 788234//cycles/311478/100,00/2/GHz > 538077//instructions/311478/100,00/0/insn per cycle > 106749//branches/311478/100,00/348/M/sec > 4556//branch-misses/311478/100,00/4/of all branches When I run `LC_NUMERIC=fr_FR.UTF-8 perf stat --field-separator "/" -- true` in my machine, I see something like: `0//cpu-migrations/779898/100.00/0.000/K/sec` which seems different from yours, it seems still using `.` as decimal separator (i.e. `100.00` and `0.000`). -- 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: us...@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a change in pull request #391: Fixed parsing of `perf` output on some locales.
qianzhangxa commented on a change in pull request #391:
URL: https://github.com/apache/mesos/pull/391#discussion_r644077753
##
File path: src/linux/perf.cpp
##
@@ -125,6 +125,12 @@ class Perf : public Process
private:
void execute()
{
+// If the locale is such that `LC_NUMERIC` uses the comma ',' as decimal
+// separator, parsing won't work - because of unexpected number of fields
+// and floating points format - so make sure it's set to `C`.
+std::map env = os::environment();
+env["LC_ALL"] = "C";
Review comment:
So here we set `LC_ALL` to `C` to make sure the output of `perf stat`
always has `.` as the decimal
separator (e.g., `100.00` instead of `100,00`) regardless user's locale,
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:
us...@infra.apache.org
[GitHub] [mesos] asekretenko edited a comment on pull request #386: Fixed a LevelDBStorage bug where positions would overflow.
asekretenko edited a comment on pull request #386: URL: https://github.com/apache/mesos/pull/386#issuecomment-852899178 P.S. Looks like those who implemented backward-incompatible fixes in their private forks will either need one more backward-incompatible upgrade with clearing the log, or to patch out this fix. Given that the tests in this PR should work with alternative approaches to fixing `encode()` as well, this should not be a big deal in either case. It might even be the case that upgrading from a straightforward 20-digit encoding to the encoding introduced by this patch is _functionally_ equivalent to simply dropping the log on a master: all the previously recorded 20-digit keys will become unreachable after an upgrade. -- 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: us...@infra.apache.org
[GitHub] [mesos] asekretenko commented on pull request #386: Fixed a LevelDBStorage bug where positions would overflow.
asekretenko commented on pull request #386: URL: https://github.com/apache/mesos/pull/386#issuecomment-852885997 @cf-natali Great job, and special thanks for the tests! -- 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: us...@infra.apache.org
[GitHub] [mesos] asekretenko merged pull request #386: Fixed a LevelDBStorage bug where positions would overflow.
asekretenko merged pull request #386: URL: https://github.com/apache/mesos/pull/386 -- 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: us...@infra.apache.org
