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/03/31 22:29:48 UTC

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

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

(Updated March 31, 2015, 4:29 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


Changes
-------

Addressed Adam's concerns.


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


Repository: mesos


Description
-------

Framework.id() extracts and returns FrameworkID from Framework.info.


Diffs (updated)
-----

  src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
  src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
  src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
  src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
  src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
  src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
  src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 

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


Testing (updated)
-------

make check

TODO: Test for upgrade path.


Thanks,

Kapil Arya

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

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

> On April 1, 2015, 5:28 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 1043
> > <https://reviews.apache.org/r/32585/diff/1-2/?file=908253#file908253line1043>
> >
> >     Hmm... I was actually thinking that this line of code can be removed now that you're always filling in the id in the FrameworkInfo by the time it gets to the slave.
> >     However, upon further inspection I see that there is a Slave::Framework::id field that we'll also need to deprecate, ensuring that Slave::Framework::info.id is set, and thus Archive::Framework::framework_info has the frameworkId.

I am not sure if I understand this. slave::Framework::id has been removed in this patch. I couldn't find `Slave::Framework` in the codebase. Can you point me to that?


- Kapil


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


On March 31, 2015, 4:29 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated March 31, 2015, 4:29 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

Posted by Adam B <ad...@mesosphere.io>.

> On April 1, 2015, 2:28 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 1043
> > <https://reviews.apache.org/r/32585/diff/1-2/?file=908253#file908253line1043>
> >
> >     Hmm... I was actually thinking that this line of code can be removed now that you're always filling in the id in the FrameworkInfo by the time it gets to the slave.
> >     However, upon further inspection I see that there is a Slave::Framework::id field that we'll also need to deprecate, ensuring that Slave::Framework::info.id is set, and thus Archive::Framework::framework_info has the frameworkId.
> 
> Kapil Arya wrote:
>     I am not sure if I understand this. slave::Framework::id has been removed in this patch. I couldn't find `Slave::Framework` in the codebase. Can you point me to that?

Right you are. Sorry, it was late and I confused `pid` with `id` and thought you only removed the comment, based on ReviewBoard's improper rendering of https://reviews.apache.org/r/32585/diff/1-2/
Both lines removed. Looks good. Dropping.


- Adam


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


On April 1, 2015, 12:34 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated April 1, 2015, 12:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32585/#review78496
-----------------------------------------------------------



src/slave/slave.cpp
<https://reviews.apache.org/r/32585/#comment127315>

    Hmm... I was actually thinking that this line of code can be removed now that you're always filling in the id in the FrameworkInfo by the time it gets to the slave.
    However, upon further inspection I see that there is a Slave::Framework::id field that we'll also need to deprecate, ensuring that Slave::Framework::info.id is set, and thus Archive::Framework::framework_info has the frameworkId.


- Adam B


On March 31, 2015, 1:29 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated March 31, 2015, 1:29 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32585/#review79146
-----------------------------------------------------------


LGTM, assuming no significant rebases.


src/master/master.hpp
<https://reviews.apache.org/r/32585/#comment128281>

    `const FrameworkID`?


- Adam B


On April 1, 2015, 12:34 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated April 1, 2015, 12:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

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

> On April 13, 2015, 1:47 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 1100-1103
> > <https://reviews.apache.org/r/32585/diff/4/?file=919351#file919351line1100>
> >
> >     Did you look back at https://reviews.apache.org/r/19176/ when doing this?
> >     
> >     What was the motivation for making this function instead of a field? Either way (1) the 'id' cannot be changed, and (2) there are two ways to access the ID: id() and info.id(). Is there any benefit I'm missing? Just seems like unnecessary churn in the code to me.
> >     
> >     Note that this change means the `Slave` and `Framework` structs now store their ID's differently as well, which is unfortunate.

I did look ar 19176 and part of the upgrade path is motivated by your earlier review :-).

For the id, I guess the alternative is to remove id() altogether and replace it with info.id() since the latter is always supposed to be valid. If this is preferred, I can create a new RR to address it.

Lastly, I am not sure if I understand what you mean by the 'Slave' struct. Do you mean `SlaveID` used for the `Slave` struct used in src/master/? I haven't looked closely at that but I can see if we can remove SlaveID from that struct as well (in favor of SlaveInfo.id()).


- Kapil


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


On April 7, 2015, 12:59 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated April 7, 2015, 12:59 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32585/#review79904
-----------------------------------------------------------



src/master/master.hpp
<https://reviews.apache.org/r/32585/#comment129498>

    Did you look back at https://reviews.apache.org/r/19176/ when doing this?
    
    What was the motivation for making this function instead of a field? Either way (1) the 'id' cannot be changed, and (2) there are two ways to access the ID: id() and info.id(). Is there any benefit I'm missing? Just seems like unnecessary churn in the code to me.
    
    Note that this change means the `Slave` and `Framework` structs now store their ID's differently as well, which is unfortunate.


- Ben Mahler


On April 7, 2015, 4:59 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated April 7, 2015, 4:59 p.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32585/#review79799
-----------------------------------------------------------

Ship it!


Needs a quick rebase before committing.

- Adam B


On April 7, 2015, 9:59 a.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32585/
> -----------------------------------------------------------
> 
> (Updated April 7, 2015, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2557
>     https://issues.apache.org/jira/browse/MESOS-2557
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Framework.id() extracts and returns FrameworkID from Framework.info.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
>   src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
>   src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
>   src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
>   src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 
> 
> Diff: https://reviews.apache.org/r/32585/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> TODO: Test for upgrade path.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

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

(Updated April 7, 2015, 12:59 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


Changes
-------

rebased; addressed Adam's comments.


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


Repository: mesos


Description
-------

Framework.id() extracts and returns FrameworkID from Framework.info.


Diffs (updated)
-----

  src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
  src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
  src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
  src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
  src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
  src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
  src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 

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


Testing
-------

make check

TODO: Test for upgrade path.


Thanks,

Kapil Arya

Re: Review Request 32585: Replaced Framework.id with Framework.id() in Master/Slave.

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

(Updated April 1, 2015, 3:34 p.m.)


Review request for mesos, Adam B and Niklas Nielsen.


Changes
-------

rebased


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


Repository: mesos


Description
-------

Framework.id() extracts and returns FrameworkID from Framework.info.


Diffs (updated)
-----

  src/master/http.cpp e1a87d646e9690e39a9e84ae383622018ce80401 
  src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d 
  src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 
  src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
  src/slave/http.cpp 5f0c39afd2fe9a89eb1df0052af8ab422306f30e 
  src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae 
  src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 

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


Testing
-------

make check

TODO: Test for upgrade path.


Thanks,

Kapil Arya