You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Bernd Mathiske <be...@mesosphere.io> on 2014/12/12 01:48:21 UTC
Review Request 28975: MESOS-2173: Consolidate all fetcher env vars
into one that holds a JSON object
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Bugs: MESOS-2173
https://issues.apache.org/jira/browse/MESOS-2173
Repository: mesos-git
Description
-------
Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
Diffs
-----
include/mesos/fetcher/fetcher.hpp PRE-CREATION
include/mesos/fetcher/fetcher.proto PRE-CREATION
src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
Diff: https://reviews.apache.org/r/28975/diff/
Testing
-------
make check
Thanks,
Bernd Mathiske
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Bernd Mathiske <be...@mesosphere.io>.
> On Dec. 23, 2014, 1:26 a.m., Adam B wrote:
> > src/launcher/fetcher.cpp, lines 285-286
> > <https://reviews.apache.org/r/28975/diff/1/?file=789894#file789894line285>
> >
> > How can fetcherInfo not have a commandinfo if it's required? I would think it would have errored in the parse function.
Wasn't sure if we double-check required field. Apparently not. Removed.
> On Dec. 23, 2014, 1:26 a.m., Adam B wrote:
> > src/launcher/fetcher.cpp, lines 290-291
> > <https://reviews.apache.org/r/28975/diff/1/?file=789894#file789894line290>
> >
> > Ditto on another required field, but maybe check that it's not empty string?
Now checking for empty string.
> On Dec. 23, 2014, 1:26 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.hpp, lines 47-49
> > <https://reviews.apache.org/r/28975/diff/1/?file=789895#file789895line47>
> >
> > Are we going to do this sometime soon too?
MESOS-2172 should deal with this one way or another.
- Bernd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/#review65900
-----------------------------------------------------------
On Dec. 11, 2014, 4:48 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28975/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 4:48 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2173
> https://issues.apache.org/jira/browse/MESOS-2173
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
>
>
> Diffs
> -----
>
> include/mesos/fetcher/fetcher.hpp PRE-CREATION
> include/mesos/fetcher/fetcher.proto PRE-CREATION
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
> src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
> src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
> src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
>
> Diff: https://reviews.apache.org/r/28975/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/#review65900
-----------------------------------------------------------
Looking good. Just the protobuf variables to snake_case, since that's how protobufs auto-generate their accessors.
include/mesos/fetcher/fetcher.proto
<https://reviews.apache.org/r/28975/#comment109176>
Protobufs use snake case, so command_info, work_directory, frameworks_home, and hadoop_home.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109171>
Might this want to be `using namespace mesos::fetcher` in the future, if there are more messages added there? Probably doesn't matter yet.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109175>
How can fetcherInfo not have a commandinfo if it's required? I would think it would have errored in the parse function.
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109178>
Ditto on another required field, but maybe check that it's not empty string?
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109181>
const string& ?
src/slave/containerizer/fetcher.hpp
<https://reviews.apache.org/r/28975/#comment109182>
Are we going to do this sometime soon too?
- Adam B
On Dec. 11, 2014, 4:48 p.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28975/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2014, 4:48 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2173
> https://issues.apache.org/jira/browse/MESOS-2173
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
>
>
> Diffs
> -----
>
> include/mesos/fetcher/fetcher.hpp PRE-CREATION
> include/mesos/fetcher/fetcher.proto PRE-CREATION
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
> src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
> src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
> src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
>
> Diff: https://reviews.apache.org/r/28975/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/#review64839
-----------------------------------------------------------
Patch looks great!
Reviews applied: [28975]
All tests passed.
- Mesos ReviewBot
On Dec. 12, 2014, 12:48 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28975/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2014, 12:48 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2173
> https://issues.apache.org/jira/browse/MESOS-2173
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
>
>
> Diffs
> -----
>
> include/mesos/fetcher/fetcher.hpp PRE-CREATION
> include/mesos/fetcher/fetcher.proto PRE-CREATION
> src/Makefile.am 86161fe7a8bdd86958d24adb74d434cd92d7dfb8
> src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
> src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
> src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
> src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
>
> Diff: https://reviews.apache.org/r/28975/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/#review66107
-----------------------------------------------------------
Ship it!
I'll get this committed after some minor cleanups!
src/Makefile.am
<https://reviews.apache.org/r/28975/#comment109468>
libmesos_la_SOURCES = \
$(MESOS_PROTO) $(CONTAINERIZER_PROTO) $(FETCHER_PROTO) $(SCHEDULER_PROTO)
src/launcher/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109462>
Newline between these please.
src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109467>
Let's move this down to the bottom now, closer to where we use it and return it. Eventually we can use some C++11 to simplify the return statement further.
src/slave/containerizer/fetcher.cpp
<https://reviews.apache.org/r/28975/#comment109463>
+2 not +4 here please, but that should all fit on one line right?
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/28975/#comment109464>
ASSERT_SOME(parse);
Here and all the other tests please!
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/28975/#comment109465>
ASSERT_SOME(fetcherInfo);
Here and all the other tests please!
src/tests/fetcher_tests.cpp
<https://reviews.apache.org/r/28975/#comment109466>
You swapped the ordering here, you want to expected value first, then the actual value:
EXPECT_EQ(directory, fetcherInfo.get().work_directory());
Here and all the other tests too please!
- Benjamin Hindman
On Dec. 25, 2014, 6:27 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28975/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2014, 6:27 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2173
> https://issues.apache.org/jira/browse/MESOS-2173
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
>
>
> Diffs
> -----
>
> include/mesos/fetcher/fetcher.hpp PRE-CREATION
> include/mesos/fetcher/fetcher.proto PRE-CREATION
> src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0
> src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
> src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
> src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
> src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
>
> Diff: https://reviews.apache.org/r/28975/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/#review66125
-----------------------------------------------------------
Patch looks great!
Reviews applied: [28975]
All tests passed.
- Mesos ReviewBot
On Dec. 26, 2014, 4:21 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28975/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2014, 4:21 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2173
> https://issues.apache.org/jira/browse/MESOS-2173
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
>
>
> Diffs
> -----
>
> include/mesos/fetcher/fetcher.hpp PRE-CREATION
> include/mesos/fetcher/fetcher.proto PRE-CREATION
> src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0
> src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
> src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
> src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
> src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
>
> Diff: https://reviews.apache.org/r/28975/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/
-----------------------------------------------------------
(Updated Dec. 25, 2014, 8:21 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Addressed review issues.
Bugs: MESOS-2173
https://issues.apache.org/jira/browse/MESOS-2173
Repository: mesos-git
Description
-------
Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
Diffs (updated)
-----
include/mesos/fetcher/fetcher.hpp PRE-CREATION
include/mesos/fetcher/fetcher.proto PRE-CREATION
src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0
src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
Diff: https://reviews.apache.org/r/28975/diff/
Testing
-------
make check
Thanks,
Bernd Mathiske
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/#review66079
-----------------------------------------------------------
Patch looks great!
Reviews applied: [28975]
All tests passed.
- Mesos ReviewBot
On Dec. 25, 2014, 6:27 a.m., Bernd Mathiske wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28975/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2014, 6:27 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2173
> https://issues.apache.org/jira/browse/MESOS-2173
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
>
>
> Diffs
> -----
>
> include/mesos/fetcher/fetcher.hpp PRE-CREATION
> include/mesos/fetcher/fetcher.proto PRE-CREATION
> src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0
> src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
> src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
> src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
> src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
>
> Diff: https://reviews.apache.org/r/28975/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bernd Mathiske
>
>
Re: Review Request 28975: MESOS-2173: Consolidate all fetcher env
vars into one that holds a JSON object
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28975/
-----------------------------------------------------------
(Updated Dec. 24, 2014, 10:27 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
Addressed review issues.
Bugs: MESOS-2173
https://issues.apache.org/jira/browse/MESOS-2173
Repository: mesos-git
Description
-------
Changed env var ensemble into one env var containing JSON, parsing this to protobuf for use. Adapted fetcher tests.
Diffs (updated)
-----
include/mesos/fetcher/fetcher.hpp PRE-CREATION
include/mesos/fetcher/fetcher.proto PRE-CREATION
src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0
src/launcher/fetcher.cpp 2f54d9e061d6b56a9b9cd857e5a908fee2847e68
src/slave/containerizer/fetcher.hpp 12b81b1065b6b0a0368ebd44326c75c1d4e4ba79
src/slave/containerizer/fetcher.cpp d702a9ccc15b21ba26fb9356aa69da15dfd5973e
src/tests/fetcher_tests.cpp 9e48392d31c886bad60abc8999fe293f55f589b0
Diff: https://reviews.apache.org/r/28975/diff/
Testing
-------
make check
Thanks,
Bernd Mathiske