You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexandra Sava <al...@gmail.com> on 2014/06/18 12:56:40 UTC
Review Request 22722: Refactor os::user method
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22722/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Refactor os::user method:
* receive an optional uid argument for which the corresponding username will be returned;
if no uid id provided, return the username of the current user
* change return type to Result<string>
* return None() and ErrnoError() on error cases rather then using LOG(FATAL) which ends the application
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 72f3e70ccc189d0a7dd613ccaf79537d3587c49c
3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp f6d79b10b7cb0df3e510a3bf6e681726f5f019f3
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1a6ce0b9a70c0334946e456696b814961e5b4018
src/cli/execute.cpp e881b64807f556b1c1774255beab95d60b6935d6
src/sched/sched.cpp 6e14f1cf78b9518a04c35f856fa2358a3d72ddcf
src/scheduler/scheduler.cpp 4ae188e4eba67c630f57863b6c5116162c92a0a1
src/tests/environment.cpp 005fc54759a4be1651f55249e4e8e1bafc2f73ce
src/tests/mesos.cpp 98a7c3895343b1bc84c13e54d12905de3b431338
src/tests/script.cpp 9f1be63e2c6538a3ae3fd8f39ad0ea1f66ee88a4
src/tests/slave_tests.cpp aaf509d1e0798928ab729704a42ec5aed18c11a2
Diff: https://reviews.apache.org/r/22722/diff/
Testing
-------
Thanks,
Alexandra Sava
Re: Review Request 22722: Refactor os::user method
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22722/#review46108
-----------------------------------------------------------
Bad patch!
Reviews applied: [22722]
Failed command: ./support/mesos-style.py
Error:
Checking 477 files using filter --filter=-,+build/class,+build/deprecated,+build/endif_comment,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/tab,+whitespace/todo
src/tests/environment.cpp:116: Lines should be <= 80 characters long [whitespace/line_length] [2]
Total errors found: 1
- Mesos ReviewBot
On June 18, 2014, 10:56 a.m., Alexandra Sava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22722/
> -----------------------------------------------------------
>
> (Updated June 18, 2014, 10:56 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Refactor os::user method:
> * receive an optional uid argument for which the corresponding username will be returned;
> if no uid id provided, return the username of the current user
> * change return type to Result<string>
> * return None() and ErrnoError() on error cases rather then using LOG(FATAL) which ends the application
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 72f3e70ccc189d0a7dd613ccaf79537d3587c49c
> 3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp f6d79b10b7cb0df3e510a3bf6e681726f5f019f3
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1a6ce0b9a70c0334946e456696b814961e5b4018
> src/cli/execute.cpp e881b64807f556b1c1774255beab95d60b6935d6
> src/sched/sched.cpp 6e14f1cf78b9518a04c35f856fa2358a3d72ddcf
> src/scheduler/scheduler.cpp 4ae188e4eba67c630f57863b6c5116162c92a0a1
> src/tests/environment.cpp 005fc54759a4be1651f55249e4e8e1bafc2f73ce
> src/tests/mesos.cpp 98a7c3895343b1bc84c13e54d12905de3b431338
> src/tests/script.cpp 9f1be63e2c6538a3ae3fd8f39ad0ea1f66ee88a4
> src/tests/slave_tests.cpp aaf509d1e0798928ab729704a42ec5aed18c11a2
>
> Diff: https://reviews.apache.org/r/22722/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexandra Sava
>
>
Re: Review Request 22722: Refactor os::user method
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22722/#review46156
-----------------------------------------------------------
Ship it!
Great! For this time, I will split this patch for you across stout and mesos.
I left some notes below for some small cleanups I did before committing this.
3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp
<https://reviews.apache.org/r/22722/#comment81388>
This should be an ASSERT because we want the test to stop if this condition is not satisfied.
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/22722/#comment81384>
This can be done in one line using EXPECT_SOME_EQ.
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/22722/#comment81386>
In general, let's avoid calling .get() on a Result without ensuring that it is SOME.
src/cli/execute.cpp
<https://reviews.apache.org/r/22722/#comment81389>
No need for std::
src/sched/sched.cpp
<https://reviews.apache.org/r/22722/#comment81390>
We should have a CHECK_SOME here at least, that will print a CHECK failure with the error reason if things fail.
src/scheduler/scheduler.cpp
<https://reviews.apache.org/r/22722/#comment81391>
Nice! I think for now we can keep the previous semantics and just use CHECK_SOME to ensure we can get the current user, as such a failure is quite serious.
src/tests/environment.cpp
<https://reviews.apache.org/r/22722/#comment81392>
Nice! Let's store the Result<string> to avoid re-computing os::user() and potentially having .get() fail.
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/22722/#comment81394>
CHECK_SOME will print the error for you! :)
- Ben Mahler
On June 18, 2014, 10:56 a.m., Alexandra Sava wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22722/
> -----------------------------------------------------------
>
> (Updated June 18, 2014, 10:56 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Refactor os::user method:
> * receive an optional uid argument for which the corresponding username will be returned;
> if no uid id provided, return the username of the current user
> * change return type to Result<string>
> * return None() and ErrnoError() on error cases rather then using LOG(FATAL) which ends the application
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 72f3e70ccc189d0a7dd613ccaf79537d3587c49c
> 3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp f6d79b10b7cb0df3e510a3bf6e681726f5f019f3
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1a6ce0b9a70c0334946e456696b814961e5b4018
> src/cli/execute.cpp e881b64807f556b1c1774255beab95d60b6935d6
> src/sched/sched.cpp 6e14f1cf78b9518a04c35f856fa2358a3d72ddcf
> src/scheduler/scheduler.cpp 4ae188e4eba67c630f57863b6c5116162c92a0a1
> src/tests/environment.cpp 005fc54759a4be1651f55249e4e8e1bafc2f73ce
> src/tests/mesos.cpp 98a7c3895343b1bc84c13e54d12905de3b431338
> src/tests/script.cpp 9f1be63e2c6538a3ae3fd8f39ad0ea1f66ee88a4
> src/tests/slave_tests.cpp aaf509d1e0798928ab729704a42ec5aed18c11a2
>
> Diff: https://reviews.apache.org/r/22722/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alexandra Sava
>
>
Re: Review Request 22722: Refactor os::user method
Posted by Alexandra Sava <al...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22722/
-----------------------------------------------------------
(Updated June 18, 2014, 10:56 a.m.)
Review request for mesos and Ben Mahler.
Repository: mesos-git
Description
-------
Refactor os::user method:
* receive an optional uid argument for which the corresponding username will be returned;
if no uid id provided, return the username of the current user
* change return type to Result<string>
* return None() and ErrnoError() on error cases rather then using LOG(FATAL) which ends the application
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 72f3e70ccc189d0a7dd613ccaf79537d3587c49c
3rdparty/libprocess/3rdparty/stout/tests/os/setns_tests.cpp f6d79b10b7cb0df3e510a3bf6e681726f5f019f3
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1a6ce0b9a70c0334946e456696b814961e5b4018
src/cli/execute.cpp e881b64807f556b1c1774255beab95d60b6935d6
src/sched/sched.cpp 6e14f1cf78b9518a04c35f856fa2358a3d72ddcf
src/scheduler/scheduler.cpp 4ae188e4eba67c630f57863b6c5116162c92a0a1
src/tests/environment.cpp 005fc54759a4be1651f55249e4e8e1bafc2f73ce
src/tests/mesos.cpp 98a7c3895343b1bc84c13e54d12905de3b431338
src/tests/script.cpp 9f1be63e2c6538a3ae3fd8f39ad0ea1f66ee88a4
src/tests/slave_tests.cpp aaf509d1e0798928ab729704a42ec5aed18c11a2
Diff: https://reviews.apache.org/r/22722/diff/
Testing
-------
Thanks,
Alexandra Sava