[GitHub] [mesos] bluezd commented on a change in pull request #353: Hook manager mutex and global variable enhancement.

2020-03-24 Thread GitBox 
bluezd commented on a change in pull request #353: Hook manager mutex and 
global variable enhancement.
URL: https://github.com/apache/mesos/pull/353#discussion_r397585853
 
 

 ##
 File path: src/hook/manager.cpp
 ##
 @@ -47,16 +47,16 @@ using mesos::modules::ModuleManager;
 namespace mesos {
 namespace internal {
 
-static std::mutex mutex;
-static LinkedHashMap availableHooks;
+static std::mutex* mutex = new(std::mutex);
+static LinkedHashMap* availableHooks = 
new(LinkedHashMap);
 
 Review comment:
   Thanks for your info, just installed the commit hook and make changes to the 
code accordingly per your suggestion.
   
   Could you help reviewing it again ? @bmahler 


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


With regards,
Apache Git Services

[GitHub] [mesos] bluezd commented on a change in pull request #353: Hook manager mutex and global variable enhancement.

2020-03-24 Thread GitBox 
bluezd commented on a change in pull request #353: Hook manager mutex and 
global variable enhancement.
URL: https://github.com/apache/mesos/pull/353#discussion_r397585853
 
 

 ##
 File path: src/hook/manager.cpp
 ##
 @@ -47,16 +47,16 @@ using mesos::modules::ModuleManager;
 namespace mesos {
 namespace internal {
 
-static std::mutex mutex;
-static LinkedHashMap availableHooks;
+static std::mutex* mutex = new(std::mutex);
+static LinkedHashMap* availableHooks = 
new(LinkedHashMap);
 
 Review comment:
   Thanks for your info, just installed the commit hook and make changes to the 
code accordingly per your suggestion.
   
   Could you help reviewing it again ?


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


With regards,
Apache Git Services

[GitHub] [mesos] bluezd commented on a change in pull request #353: hook manager mutex and global variable enhancement

2020-03-24 Thread GitBox 
bluezd commented on a change in pull request #353: hook manager mutex and 
global variable enhancement
URL: https://github.com/apache/mesos/pull/353#discussion_r397585853
 
 

 ##
 File path: src/hook/manager.cpp
 ##
 @@ -47,16 +47,16 @@ using mesos::modules::ModuleManager;
 namespace mesos {
 namespace internal {
 
-static std::mutex mutex;
-static LinkedHashMap availableHooks;
+static std::mutex* mutex = new(std::mutex);
+static LinkedHashMap* availableHooks = 
new(LinkedHashMap);
 
 Review comment:
   Thanks for your info, just installed the commit hook and make changes to the 
code accordingly per your suggestion.


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


With regards,
Apache Git Services

Re: Review Request 72262: Added resource limits to v0 endpoint results.

2020-03-24 Thread Qian Zhang 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72262/#review220076
---


Ship it!




Ship It!

- Qian Zhang


On March 25, 2020, 4:48 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72262/
> ---
> 
> (Updated March 25, 2020, 4:48 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10087
> https://issues.apache.org/jira/browse/MESOS-10087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added resource limits to v0 endpoint results.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
>   src/common/http.cpp 3dd77dc826d2b1606a78c42ff0fac5794f2a3805 
>   src/tests/common/http_tests.cpp 5f3652732641f763343f98651dab2472131b38e3 
>   src/tests/master_tests.cpp 617abfa0a97f27c8cbe6aa6eb5345c5c07d3097f 
> 
> 
> Diff: https://reviews.apache.org/r/72262/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72262: Added resource limits to v0 endpoint results.

2020-03-24 Thread Mesos Reviewbot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72262/#review220074
---



Patch looks great!

Reviews applied: [72262]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On March 24, 2020, 8:48 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72262/
> ---
> 
> (Updated March 24, 2020, 8:48 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10087
> https://issues.apache.org/jira/browse/MESOS-10087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added resource limits to v0 endpoint results.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
>   src/common/http.cpp 3dd77dc826d2b1606a78c42ff0fac5794f2a3805 
>   src/tests/common/http_tests.cpp 5f3652732641f763343f98651dab2472131b38e3 
>   src/tests/master_tests.cpp 617abfa0a97f27c8cbe6aa6eb5345c5c07d3097f 
> 
> 
> Diff: https://reviews.apache.org/r/72262/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72262: Added resource limits to v0 endpoint results.

2020-03-24 Thread Greg Mann 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72262/
---

(Updated March 24, 2020, 8:48 p.m.)


Review request for mesos and Qian Zhang.


Bugs: MESOS-10087
https://issues.apache.org/jira/browse/MESOS-10087


Repository: mesos


Description
---

Added resource limits to v0 endpoint results.


Diffs (updated)
-

  src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
  src/common/http.cpp 3dd77dc826d2b1606a78c42ff0fac5794f2a3805 
  src/tests/common/http_tests.cpp 5f3652732641f763343f98651dab2472131b38e3 
  src/tests/master_tests.cpp 617abfa0a97f27c8cbe6aa6eb5345c5c07d3097f 


Diff: https://reviews.apache.org/r/72262/diff/2/

Changes: https://reviews.apache.org/r/72262/diff/1-2/


Testing
---

`make check`


Thanks,

Greg Mann

[GitHub] [mesos] bmahler commented on issue #354: Avoid error on mesos logs upon docker image fetch

2020-03-24 Thread GitBox 
bmahler commented on issue #354: Avoid error on mesos logs upon docker image 
fetch
URL: https://github.com/apache/mesos/pull/354#issuecomment-603477873
 
 
   Thanks @kamaradclimber, we chatted a bit in slack about this since it looked 
a bit surprising to me:
   
   https://mesos.slack.com/archives/C1YN31DPA/p1584988424011600
   
   One of us will get to this shortly.


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


With regards,
Apache Git Services

[GitHub] [mesos] bmahler commented on a change in pull request #353: hook manager mutex and global variable enhancement

2020-03-24 Thread GitBox 
bmahler commented on a change in pull request #353: hook manager mutex and 
global variable enhancement
URL: https://github.com/apache/mesos/pull/353#discussion_r397426753
 
 

 ##
 File path: src/hook/manager.cpp
 ##
 @@ -47,16 +47,16 @@ using mesos::modules::ModuleManager;
 namespace mesos {
 namespace internal {
 
-static std::mutex mutex;
-static LinkedHashMap availableHooks;
+static std::mutex* mutex = new(std::mutex);
+static LinkedHashMap* availableHooks = 
new(LinkedHashMap);
 
 Review comment:
   This line is also > 80 characters. Are you using the commit hook? It would 
catch this issue:
   
   
http://mesos.apache.org/documentation/latest/advanced-contribution/#create-your-patch


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


With regards,
Apache Git Services

[GitHub] [mesos] bmahler commented on a change in pull request #353: hook manager mutex and global variable enhancement

2020-03-24 Thread GitBox 
bmahler commented on a change in pull request #353: hook manager mutex and 
global variable enhancement
URL: https://github.com/apache/mesos/pull/353#discussion_r397425354
 
 

 ##
 File path: src/hook/manager.cpp
 ##
 @@ -47,16 +47,16 @@ using mesos::modules::ModuleManager;
 namespace mesos {
 namespace internal {
 
-static std::mutex mutex;
-static LinkedHashMap availableHooks;
+static std::mutex* mutex = new(std::mutex);
+static LinkedHashMap* availableHooks = 
new(LinkedHashMap);
 
 Review comment:
   I'm not familiar with this `new` syntax, we would use:
   
   ```
   static std::mutex* mutex = new std::mutex();
   static LinkedHashMap* availableHooks = new 
LinkedHashMap();
   ```


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


With regards,
Apache Git Services

[GitHub] [mesos] bmahler commented on issue #353: hook manager mutex and global variable enhancement

2020-03-24 Thread GitBox 
bmahler commented on issue #353: hook manager mutex and global variable 
enhancement
URL: https://github.com/apache/mesos/pull/353#issuecomment-603473389
 
 
   @asekretenko these are good questions to be asking, as I also felt this 
looked a bit strange. I suspect it's just modeled in the same way that 
ModuleManager is written which has quite a bit more to it but is also an all 
static class with similar static non-POD issues. For unload, it looks like it 
is used for testing so I'm not sure how easily it could be removed. At this 
point I haven't read through all the module related code enough to be 
contemplating changing the design, so I can't write a useful TODO or ticket 
(but please feel free to if you're interested in pitching in to clean this up!)
   
   Since @bluezd is a new contributor and was interested in hooks/modules, I 
looked for some small tickets for him to get some experience with the 
contribution process, and so this patch is just focusing on the trivial work of 
removing static non-PODs (which we ban) and fixing some unsafe locking in this 
code.


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


With regards,
Apache Git Services

Re: Review Request 72263: Moved containerizer utils in CMakeLists.

2020-03-24 Thread Greg Mann 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72263/#review220072
---


Ship it!




Ship It!

- Greg Mann


On March 24, 2020, 8:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72263/
> ---
> 
> (Updated March 24, 2020, 8:40 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is to ensure the function `calculateOOMScoreAdj()` can be resolved
> on Windows.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 5133550561ace4c2a6dfbdae2e333c921eb531cb 
> 
> 
> Diff: https://reviews.apache.org/r/72263/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 72263: Moved containerizer utils in CMakeLists.

2020-03-24 Thread Mesos Reviewbot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72263/#review220067
---



Patch looks great!

Reviews applied: [72263]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On March 24, 2020, 8:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72263/
> ---
> 
> (Updated March 24, 2020, 8:40 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-10048
> https://issues.apache.org/jira/browse/MESOS-10048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is to ensure the function `calculateOOMScoreAdj()` can be resolved
> on Windows.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 5133550561ace4c2a6dfbdae2e333c921eb531cb 
> 
> 
> Diff: https://reviews.apache.org/r/72263/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 72263: Moved containerizer utils in CMakeLists.

2020-03-24 Thread Qian Zhang 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72263/
---

Review request for mesos and Greg Mann.


Bugs: MESOS-10048
https://issues.apache.org/jira/browse/MESOS-10048


Repository: mesos


Description
---

This is to ensure the function `calculateOOMScoreAdj()` can be resolved
on Windows.


Diffs
-

  src/CMakeLists.txt 5133550561ace4c2a6dfbdae2e333c921eb531cb 


Diff: https://reviews.apache.org/r/72263/diff/1/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72262: Added resource limits to v0 endpoint results.

2020-03-24 Thread Qian Zhang 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72262/#review220065
---




src/tests/common/http_tests.cpp
Lines 115-119 (patched)


I think it should be something like:
```
  "  \"limits\":"
  "  {"
  "\"cpus\":1.0,"
  "\"mem\":32"
  "  },"
```
so that the format of limits will be consistent with the format of 
resources:
```
  "  \"resources\":"
  "  {"
  "\"cpus\":0,"
  "\"disk\":0,"
  "\"gpus\":0,"
  "\"mem\":0"
  "  },"
```

Actually after applying this patch, what I get from the master's `/tasks` 
endpoint is like the below which I think is correct. So that's why I think we 
should have the same result in this test, which means we may need to change the 
implementation of the function `JSON::Object model(const 
google::protobuf::Map& map)`.
```
 "resources": {
"disk": 0,
"mem": 128,
"gpus": 0,
"cpus": 1
  },
  "limits": {
"mem": 256,
"cpus": 2
  },
```


- Qian Zhang


On March 24, 2020, 11:54 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72262/
> ---
> 
> (Updated March 24, 2020, 11:54 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10087
> https://issues.apache.org/jira/browse/MESOS-10087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added resource limits to v0 endpoint results.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
>   src/common/http.cpp 3dd77dc826d2b1606a78c42ff0fac5794f2a3805 
>   src/tests/common/http_tests.cpp 5f3652732641f763343f98651dab2472131b38e3 
>   src/tests/master_tests.cpp 617abfa0a97f27c8cbe6aa6eb5345c5c07d3097f 
> 
> 
> Diff: https://reviews.apache.org/r/72262/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 72262: Added resource limits to v0 endpoint results.

2020-03-24 Thread Mesos Reviewbot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72262/#review220064
---



Patch looks great!

Reviews applied: [72262]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On March 24, 2020, 3:54 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72262/
> ---
> 
> (Updated March 24, 2020, 3:54 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-10087
> https://issues.apache.org/jira/browse/MESOS-10087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added resource limits to v0 endpoint results.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 02633e175c0848ee622cb5108a2e18db5e030641 
>   src/common/http.cpp 3dd77dc826d2b1606a78c42ff0fac5794f2a3805 
>   src/tests/common/http_tests.cpp 5f3652732641f763343f98651dab2472131b38e3 
>   src/tests/master_tests.cpp 617abfa0a97f27c8cbe6aa6eb5345c5c07d3097f 
> 
> 
> Diff: https://reviews.apache.org/r/72262/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>