mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Schlicht <...@mesosphere.io>
Subject Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.
Date Wed, 25 May 2016 09:45:13 GMT

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



Thanks for taking this on!
Please keep in mind that to work around the inconsistency, we created local copies or references
of member variables of `this`. With that we were able to use it in the lambda and/or the (static)
continuation. With the inconsistency gone, we're able to access `this` in the lambda/continuation,
meaning that the local copies are now obsolete. Please remove them from the existing code.


src/slave/http.cpp (line 369)
<https://reviews.apache.org/r/47822/#comment199635>

    There's no need for this here anymore, because it was necessary due to the inconsistent
behavior you're fixing. You can now use `slave->flags` directly in the lambda.



src/slave/http.cpp (line 384)
<https://reviews.apache.org/r/47822/#comment199636>

    s/slaveFlags/slave->flags/, see comment above.



src/slave/http.cpp (line 651)
<https://reviews.apache.org/r/47822/#comment199639>

    Remove this, same rationale as the comments in `Slave::Http::flags`.



src/slave/http.cpp (line 661)
<https://reviews.apache.org/r/47822/#comment199638>

    Same as in `Slave::Http::flags`: s/pid/slave->self()/ and use members of `this` directly.



src/slave/http.cpp (lines 668 - 669)
<https://reviews.apache.org/r/47822/#comment199640>

    s/pid/slave->self()/



src/slave/http.cpp (line 769)
<https://reviews.apache.org/r/47822/#comment199643>

    Can also be removed.



src/slave/http.cpp (lines 771 - 773)
<https://reviews.apache.org/r/47822/#comment199644>

    Remove this.



src/slave/http.cpp (line 777)
<https://reviews.apache.org/r/47822/#comment199642>

    s/pid/slave->self()/



src/slave/http.cpp (line 783)
<https://reviews.apache.org/r/47822/#comment199646>

    s/localSlave/slave/


- Jan Schlicht


On May 25, 2016, 11:23 a.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47822/
> -----------------------------------------------------------
> 
> (Updated May 25, 2016, 11:23 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, Benjamin Bannier,
Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5293
>     https://issues.apache.org/jira/browse/MESOS-5293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The master and agent code for routing endpoints are made
> consistent. Now they both capture 'this' variable in the
> lambda calls in routing function arguments. Continuations
> are no more need to be static.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47822/diff/
> 
> 
> Testing
> -------
> 
> Unit tests.
> 
> On ubuntu 16.04:
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> Manual testing.
> 
> 1. Ran master with:
> 
> 
>    sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> 
> 2. ACL File:
> 
> 
>   {
>     "get_endpoints": [
>       {
>         "principals": { "type": "NONE" },
>         "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
>       }
>     ]
>   } 
> 
> 
> 3. Ran slave with:
> 
> 
>    sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 --acls=file:///home/abhishek/testAcl
> 
> 
> 4. Ran toy-framework with:
> 
> 
>    sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo hello"
> 
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP error 403:
Forbidden
> 
> 6. Changed ACL to:
> 
> 
>   {
>     "get_endpoints": [
>       {
>         "principals": { "type": "ANY" },
>         "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
>       }
>     ]
>   }
> 
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> 
> 
>     [{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
Executor (Task: 699) (Command: sh -c 'echo hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-0000","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",.......
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message