You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2015/01/06 00:24:06 UTC

Review Request 29601: Removed slave/state.hpp dependency on isolator.hpp.

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

Review request for mesos.


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


Repository: mesos-git


Description
-------

[4/9] Build Mesos Modules outside Mesos source tree.

This is to make isolator.hpp independent of any other files in src/.  There is one minor glitch with this approach. Apparently, isolator.hpp requires RunState and TaskState structs that were previously declared inside state.hpp.  The solution was to move these structs from state.hpp to isolator.hpp.


Diffs
-----

  src/slave/containerizer/isolator.hpp e82a14201f734292a46b8816c894fa8c766dae5a 
  src/slave/state.hpp 592182b21ae2700c627f180db38ca5dc1b3c9e8a 

Diff: https://reviews.apache.org/r/29601/diff/


Testing
-------

make check


Thanks,

Kapil Arya

Re: Review Request 29601: Removed slave/state.hpp dependency on isolator.hpp.

Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29601/
-----------------------------------------------------------

(Updated Jan. 9, 2015, 6:12 p.m.)


Review request for mesos, Niklas Nielsen, Till Toenshoff, and Timothy Chen.


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


Repository: mesos-git


Description
-------

[4/9] Build Mesos Modules outside Mesos source tree.

This is to make isolator.hpp independent of any other files in src/.  There is one minor glitch with this approach. Apparently, isolator.hpp requires RunState and TaskState structs that were previously declared inside state.hpp.  The solution was to move these structs from state.hpp to isolator.hpp.


Diffs
-----

  src/slave/containerizer/isolator.hpp e82a14201f734292a46b8816c894fa8c766dae5a 
  src/slave/state.hpp 592182b21ae2700c627f180db38ca5dc1b3c9e8a 

Diff: https://reviews.apache.org/r/29601/diff/


Testing
-------

make check


Thanks,

Kapil Arya

Re: Review Request 29601: Removed slave/state.hpp dependency on isolator.hpp.

Posted by Kapil Arya <ka...@mesosphere.io>.

> On Jan. 6, 2015, 6:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/isolator.hpp, line 42
> > <https://reviews.apache.org/r/29601/diff/1/?file=807143#file807143line42>
> >
> >     I think this can become pretty confusing, both maintainability and semantically.
> >     
> >     If somehow someone else modifying isolator.hpp decides to use another struct from state.hpp, technically he/she won't need to move the structs here and it will break the modules.
> >     
> >     Also it's strange to include slave/containerizer/isolator.hpp just to use TaskState and SlaveState as well.
> >     
> >     Is there a way to expose slave/state.hpp instead?

It is possible to expose slave/state.hpp but I was concerned about the namespace "mesos::internal::slave::state" and wasn't sure about the namespace policy for public headers.

I do know that isolator.hpp also includes a "mesos::internal::slave::state" namespace.  Can you suggest a solution?


- Kapil


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


On Jan. 6, 2015, 5:39 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29601/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 5:39 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2096
>     https://issues.apache.org/jira/browse/MESOS-2096
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> [4/9] Build Mesos Modules outside Mesos source tree.
> 
> This is to make isolator.hpp independent of any other files in src/.  There is one minor glitch with this approach. Apparently, isolator.hpp requires RunState and TaskState structs that were previously declared inside state.hpp.  The solution was to move these structs from state.hpp to isolator.hpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/isolator.hpp e82a14201f734292a46b8816c894fa8c766dae5a 
>   src/slave/state.hpp 592182b21ae2700c627f180db38ca5dc1b3c9e8a 
> 
> Diff: https://reviews.apache.org/r/29601/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 29601: Removed slave/state.hpp dependency on isolator.hpp.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29601/#review66947
-----------------------------------------------------------



src/slave/containerizer/isolator.hpp
<https://reviews.apache.org/r/29601/#comment110679>

    I think this can become pretty confusing, both maintainability and semantically.
    
    If somehow someone else modifying isolator.hpp decides to use another struct from state.hpp, technically he/she won't need to move the structs here and it will break the modules.
    
    Also it's strange to include slave/containerizer/isolator.hpp just to use TaskState and SlaveState as well.
    
    Is there a way to expose slave/state.hpp instead?


- Timothy Chen


On Jan. 6, 2015, 10:39 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29601/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 10:39 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-2096
>     https://issues.apache.org/jira/browse/MESOS-2096
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> [4/9] Build Mesos Modules outside Mesos source tree.
> 
> This is to make isolator.hpp independent of any other files in src/.  There is one minor glitch with this approach. Apparently, isolator.hpp requires RunState and TaskState structs that were previously declared inside state.hpp.  The solution was to move these structs from state.hpp to isolator.hpp.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/isolator.hpp e82a14201f734292a46b8816c894fa8c766dae5a 
>   src/slave/state.hpp 592182b21ae2700c627f180db38ca5dc1b3c9e8a 
> 
> Diff: https://reviews.apache.org/r/29601/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 29601: Removed slave/state.hpp dependency on isolator.hpp.

Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29601/
-----------------------------------------------------------

(Updated Jan. 6, 2015, 5:39 p.m.)


Review request for mesos, Niklas Nielsen, Till Toenshoff, and Timothy Chen.


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


Repository: mesos-git


Description
-------

[4/9] Build Mesos Modules outside Mesos source tree.

This is to make isolator.hpp independent of any other files in src/.  There is one minor glitch with this approach. Apparently, isolator.hpp requires RunState and TaskState structs that were previously declared inside state.hpp.  The solution was to move these structs from state.hpp to isolator.hpp.


Diffs
-----

  src/slave/containerizer/isolator.hpp e82a14201f734292a46b8816c894fa8c766dae5a 
  src/slave/state.hpp 592182b21ae2700c627f180db38ca5dc1b3c9e8a 

Diff: https://reviews.apache.org/r/29601/diff/


Testing
-------

make check


Thanks,

Kapil Arya

Re: Review Request 29601: Removed slave/state.hpp dependency on isolator.hpp.

Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29601/
-----------------------------------------------------------

(Updated Jan. 5, 2015, 6:27 p.m.)


Review request for mesos, Niklas Nielsen and Till Toenshoff.


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


Repository: mesos-git


Description
-------

[4/9] Build Mesos Modules outside Mesos source tree.

This is to make isolator.hpp independent of any other files in src/.  There is one minor glitch with this approach. Apparently, isolator.hpp requires RunState and TaskState structs that were previously declared inside state.hpp.  The solution was to move these structs from state.hpp to isolator.hpp.


Diffs
-----

  src/slave/containerizer/isolator.hpp e82a14201f734292a46b8816c894fa8c766dae5a 
  src/slave/state.hpp 592182b21ae2700c627f180db38ca5dc1b3c9e8a 

Diff: https://reviews.apache.org/r/29601/diff/


Testing
-------

make check


Thanks,

Kapil Arya