You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2014/04/02 19:32:17 UTC
Review Request 19948: Added os::environment.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/
-----------------------------------------------------------
Review request for mesos, Dominic Hamon and Vinod Kone.
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
Diff: https://reviews.apache.org/r/19948/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 19948: Added os::environment.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39313
-----------------------------------------------------------
Ship it!
Ship It!
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/19948/#comment71658>
an alternative test to this would be to setenv whatever test cases you are interested in and check specifically for those.
- Dominic Hamon
On April 2, 2014, 10:52 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 10:52 a.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39338
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 2, 2014, 7:38 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 7:38 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 883926be92cada1ab93441fcbe9c4d93fe7b39ff
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39337
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/19948/#comment71688>
this isn't necessary any more
- Dominic Hamon
On April 2, 2014, 12:38 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 12:38 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 883926be92cada1ab93441fcbe9c4d93fe7b39ff
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/
-----------------------------------------------------------
(Updated April 2, 2014, 7:38 p.m.)
Review request for mesos, Dominic Hamon and Vinod Kone.
Changes
-------
Updates based on review comments.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 883926be92cada1ab93441fcbe9c4d93fe7b39ff
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
Diff: https://reviews.apache.org/r/19948/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 19948: Added os::environment.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39319
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19948]
All tests passed.
- Mesos ReviewBot
On April 2, 2014, 5:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 5:52 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Ben Mahler <be...@gmail.com>.
> On April 2, 2014, 7:07 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 114
> > <https://reviews.apache.org/r/19948/diff/4/?file=546009#file546009line114>
> >
> > why not hashmap?
I would imagine std::map to be a bit better than hashmap here since we may want to be dealing with the environment in alphabetical order. For example, printing to output, or using in a command string 'A=1 B=2 echo hi'. In both these cases humans will often find it easier to read these if they are in alphabetical order.
Since there is no meaning in the order of the environment, we should either leave the order as is, or use a well-defined order (alphabetical). Re-ordering in a poorly-defined way (hash value) seems counter-productive since I suspect there will be iteration over the environment in many places in the code.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39330
-----------------------------------------------------------
On April 2, 2014, 7:38 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 7:38 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 883926be92cada1ab93441fcbe9c4d93fe7b39ff
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39330
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/19948/#comment71677>
why not hashmap?
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/19948/#comment71680>
Can you also take this opportunity to cleanup FlagsBase::extract() to use os::environment() instead of os::environ()? That's the only place where it is used afaict.
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/19948/#comment71679>
this could be
EXPECT_TRUE(environment.contains(key)) if os::environment returns a hashmap.
- Vinod Kone
On April 2, 2014, 5:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 5:52 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/
-----------------------------------------------------------
(Updated April 2, 2014, 5:52 p.m.)
Review request for mesos, Dominic Hamon and Vinod Kone.
Changes
-------
Updates from review comments.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
Diff: https://reviews.apache.org/r/19948/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 19948: Added os::environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/
-----------------------------------------------------------
(Updated April 2, 2014, 5:51 p.m.)
Review request for mesos, Dominic Hamon and Vinod Kone.
Changes
-------
Updates from review comments.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
Diff: https://reviews.apache.org/r/19948/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 19948: Added os::environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/
-----------------------------------------------------------
(Updated April 2, 2014, 5:50 p.m.)
Review request for mesos, Dominic Hamon and Vinod Kone.
Changes
-------
Updates from review comments.
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
Diff: https://reviews.apache.org/r/19948/diff/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 19948: Added os::environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 2, 2014, 5:35 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 123
> > <https://reviews.apache.org/r/19948/diff/1/?file=545964#file545964line123>
> >
> > this isn't necessarily malformed:
> >
> > MY_FILTER='--gtest_filter=Metric*'
> >
> >
Of course, thanks! I'll update the code and add a test.
> On April 2, 2014, 5:35 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 297
> > <https://reviews.apache.org/r/19948/diff/1/?file=545965#file545965line297>
> >
> > needed for this review?
I added 'using std::vector' so it's just housekeeping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39301
-----------------------------------------------------------
On April 2, 2014, 5:32 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 5:32 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 2, 2014, 5:35 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 76
> > <https://reviews.apache.org/r/19948/diff/1/?file=545965#file545965line76>
> >
> > const map<string, string>& ?
I use the '[]' operator which requires non-const.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39301
-----------------------------------------------------------
On April 2, 2014, 5:52 p.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 5:52 p.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 19948: Added os::environment.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19948/#review39301
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/19948/#comment71646>
this isn't necessarily malformed:
MY_FILTER='--gtest_filter=Metric*'
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/19948/#comment71647>
const map<string, string>& ?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/19948/#comment71648>
needed for this review?
- Dominic Hamon
On April 2, 2014, 10:32 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19948/
> -----------------------------------------------------------
>
> (Updated April 2, 2014, 10:32 a.m.)
>
>
> Review request for mesos, Dominic Hamon and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 901e5550c82c12934a6b9c3154f030c677e41a38
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 1babc1892dff0026c5d12178616a7a8457858eaa
>
> Diff: https://reviews.apache.org/r/19948/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>