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

[Kapil Arya]

> On April 13, 2015, 1:47 p.m., Ben Mahler wrote:
> > src/master/master.hpp, lines 1100-1103
> > 
> >
> > 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.

[Ben Mahler]

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



src/master/master.hpp


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.

[Adam B]

---
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.

[Kapil Arya]

---
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.

[Adam B]

---
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


`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.

[Adam B]

> On April 1, 2015, 2:28 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 1043
> > 
> >
> > 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.

[Kapil Arya]

---
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

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

[Kapil Arya]

> On April 1, 2015, 5:28 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 1043
> > 
> >
> > 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.

[Adam B]

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



src/slave/slave.cpp


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.

[Kapil Arya]

---
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