You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2015/05/05 00:21:11 UTC
Review Request 33823: Protocol file with the HTTP API messages between
executor and slave
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/
-----------------------------------------------------------
Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
Repository: mesos
Description
-------
See summary.
Diffs
-----
include/mesos/executor/executor.hpp PRE-CREATION
include/mesos/executor/executor.proto PRE-CREATION
src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
Diff: https://reviews.apache.org/r/33823/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review84529
-----------------------------------------------------------
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135827>
s/Low-level executor/Executor/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135828>
Similar to what we did in scheduler.proto, please have a pointer to the messages below.
e.g.,
SUBSCRIBED = 1; // See 'Subscribed' below.
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135838>
s/ACKNOWLEDGE/ACKNOWLEDGED/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135830>
'executor_id' is a required field in ExecutorInfo. So it will be set. Why did you mention it specifically here?
Also, note that these are events received by the executor. So any expectations "will be" set not "must be" set.
Just say that, 'id' field will be set in 'framework_info'.
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135831>
s/Once received/Once successfully launched/
s/an 'Update'/a TASK_RUNNING 'Update'/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135832>
s/received/the task is terminated/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135833>
s/scheduler/slave/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135834>
s/are also/should also be/
s/it needs to/it re-subscribes/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135839>
s/Acknowledge/Acknowledged/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135835>
s/case of/case/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135836>
s/scheduler/executor/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135840>
s/Acknowledge/Acknowledged/
s/acknowledge/acknowledged/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135841>
s/Low-level scheduler/Scheduler/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135842>
Similar to events, have pointers to messages below
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135864>
s/Acknowlede/Acknowledged/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135867>
Put this above Update message.
s/Request the slave to be resubscribed/Request to resubscribe with the slave/
s/It includes/It must include/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment135870>
Kill this.
- Vinod Kone
On May 20, 2015, 6:41 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 20, 2015, 6:41 a.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review85627
-----------------------------------------------------------
Ship it!
Hey @vinod - I think Alex made all the suggested fixes: if you are happy with this patch, could you please help him out to commit?
Thanks!
- Marco Massenzio
On May 21, 2015, 1:11 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 1:11 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review85759
-----------------------------------------------------------
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment137514>
s/an/a , also can we try to be consistent with either 'Update' or update through all the comments when refering to the tasks status enum?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment137515>
see comment above
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment137516>
see comment above, should we add '(seee TaskState in mesos.proto)' in the first comment we refer to it?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment137525>
add a comment that points to the 'Events' section above
- Isabel Jimenez
On May 21, 2015, 1:11 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 1:11 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review86232
-----------------------------------------------------------
Ship it!
Ship It!
- Isabel Jimenez
On June 2, 2015, 12:12 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 12:12 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am a5a7306b1ef65ca2b643653779ab76c26dbb5c90
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/
-----------------------------------------------------------
(Updated June 2, 2015, 2:12 p.m.)
Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
Changes
-------
changes after Vinod's and Isabel's comments.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/executor/executor.hpp PRE-CREATION
include/mesos/executor/executor.proto PRE-CREATION
src/Makefile.am a5a7306b1ef65ca2b643653779ab76c26dbb5c90
Diff: https://reviews.apache.org/r/33823/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review85754
-----------------------------------------------------------
Ship it!
include/mesos/executor/executor.hpp
<https://reviews.apache.org/r/33823/#comment137503>
s/this/This/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment137501>
s/an/a/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment137502>
What do you mean by this? I think you wanted this on #164 not here.
- Vinod Kone
On May 21, 2015, 1:11 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 21, 2015, 1:11 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/
-----------------------------------------------------------
(Updated May 21, 2015, 3:11 p.m.)
Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
Changes
-------
Documentation changes.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/executor/executor.hpp PRE-CREATION
include/mesos/executor/executor.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/33823/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/
-----------------------------------------------------------
(Updated May 20, 2015, 8:41 a.m.)
Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
Changes
-------
Added documentation, changes from reviewers.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/executor/executor.hpp PRE-CREATION
include/mesos/executor/executor.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/33823/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/
-----------------------------------------------------------
(Updated May 19, 2015, 2:19 p.m.)
Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
Changes
-------
Changes after reviews.
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
include/mesos/executor/executor.hpp PRE-CREATION
include/mesos/executor/executor.proto PRE-CREATION
src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
Diff: https://reviews.apache.org/r/33823/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review83128
-----------------------------------------------------------
include/mesos/executor/executor.hpp
<https://reviews.apache.org/r/33823/#comment133995>
not sure whether this is common practice in Mesos - if so, please feel free to ignore this comment.
I find it confusing that this include file is named exactly as /mesos/executor.hpp - there is nothing (for the uninitiated) to indicate that this is just a 'shell' around the Protobuf generated include: not the location, not the name, not nothing - and it has the same name as a completely different include.
Again, if this is the way it is, so be it - if not, please consider renaming it something more meaningful?
- Marco Massenzio
On May 4, 2015, 10:21 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 4, 2015, 10:21 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
> On May 5, 2015, 10:42 p.m., Vinod Kone wrote:
> > include/mesos/executor/executor.proto, line 48
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line48>
> >
> > Is FrameworkID not present in FrameworkInfo?
It is but it is optional. Should we instead expect it to be there and fail if not?
> On May 5, 2015, 10:42 p.m., Vinod Kone wrote:
> > include/mesos/executor/executor.proto, line 50
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line50>
> >
> > Ditto. SlaveID should already be in SlaveInfo?
Same as above. Both are supposedly available after registration, so technically not needed here.
> On May 5, 2015, 10:42 p.m., Vinod Kone wrote:
> > include/mesos/executor/executor.proto, lines 60-61
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line60>
> >
> > Why SlaveID and FrameworkID?
They are required by `ExecutorProcess::statusUpdateAcknowledgement`; though only FrameworkId is used and only to log.
> On May 5, 2015, 10:42 p.m., Vinod Kone wrote:
> > include/mesos/executor/executor.proto, line 117
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line117>
> >
> > No corresponding Type for this?
> >
> > Also, how and when is this used?
I had problems with the naming and I can use your help. The core issue here is, the message is use for resubscription. It just adds the list of unacknowledged messages and that is the difference with the subscription message which is mostly empty, any idea for a name?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review82545
-----------------------------------------------------------
On May 5, 2015, 12:21 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 12:21 a.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
> On May 5, 2015, 10:42 p.m., Vinod Kone wrote:
> > include/mesos/executor/executor.proto, line 117
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line117>
> >
> > No corresponding Type for this?
> >
> > Also, how and when is this used?
>
> Alexander Rojas wrote:
> I had problems with the naming and I can use your help. The core issue here is, the message is use for resubscription. It just adds the list of unacknowledged messages and that is the difference with the subscription message which is mostly empty, any idea for a name?
Now called resubscribe.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review82545
-----------------------------------------------------------
On May 20, 2015, 8:41 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 20, 2015, 8:41 a.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review82545
-----------------------------------------------------------
Can you add comments to the protobufs as we did for scheduler proto?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133293>
s/RUN/LAUNCH/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133288>
Is FrameworkID not present in FrameworkInfo?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133289>
Ditto. SlaveID should already be in SlaveInfo?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133290>
Why FrameworkInfo?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133291>
Why SlaveID and FrameworkID?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133314>
Ditto. Kill these.
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133315>
Kill these?
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133317>
s/StatusUpdate/Update/
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133318>
No corresponding Type for this?
Also, how and when is this used?
- Vinod Kone
On May 4, 2015, 10:21 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 4, 2015, 10:21 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On May 9, 2015, 2:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.hpp, line 22
> > <https://reviews.apache.org/r/33823/diff/1/?file=949197#file949197line22>
> >
> > please avoid SCREAMING COMMENTS :)
> > (also, not sure what is the use of that comment? is it a TODO? should we have a #pragma? what?)
>
> Alexander Rojas wrote:
> 1. It is a warning.
> 2. The purpose of this header is to be a reflection of `include/mesos/scheduler/scheduler.hpp`, and so they both have the same comments, in fact they look mostly identical which follows the maxim of being consistent.
Well, two wrongs don't make one right.
Maybe fix both, or at least let's not perpetuate something that maybe was not ideal to start with.
Please change to something that reads like:
// NOTE: this header only becomes valid after running protoc
// and generating the equivalent .ph.h files
// See: src/messages/mesos.proto
(or whatever the proto file is)
or something to that effect.
SCREAMING COMMENTS ARE BAD FORM !!!!!!
:-)
(you could almost hear me screaming, could you? :)))
> On May 9, 2015, 2:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.proto, line 80
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line80>
> >
> > `message` is becoming a truly overloaded term here: as a Protobuf message, the Message class, this (string) variable here, the (Message) `message` field below... how about `errorMessage` instead?
>
> Alexander Rojas wrote:
> It is called like that, because it is forwarded from the master and to the master. While I agree with you in principle, you know we like consistency. Why not discuss it in the HTTP API meeting?
I don't understand the "consistency" reference. This is an `error message` and should be named as such? what is not "consistent" about that?
When we use the same term with (semantically) different meanings, we make our code more difficult to read and understand - people have to go back and forth - especially when, as in this case, comments are very few and far between.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review83125
-----------------------------------------------------------
On May 19, 2015, 12:19 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 19, 2015, 12:19 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
> On May 9, 2015, 4:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.hpp, line 22
> > <https://reviews.apache.org/r/33823/diff/1/?file=949197#file949197line22>
> >
> > please avoid SCREAMING COMMENTS :)
> > (also, not sure what is the use of that comment? is it a TODO? should we have a #pragma? what?)
1. It is a warning.
2. The purpose of this header is to be a reflection of `include/mesos/scheduler/scheduler.hpp`, and so they both have the same comments, in fact they look mostly identical which follows the maxim of being consistent.
> On May 9, 2015, 4:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.proto, line 80
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line80>
> >
> > `message` is becoming a truly overloaded term here: as a Protobuf message, the Message class, this (string) variable here, the (Message) `message` field below... how about `errorMessage` instead?
It is called like that, because it is forwarded from the master and to the master. While I agree with you in principle, you know we like consistency. Why not discuss it in the HTTP API meeting?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review83125
-----------------------------------------------------------
On May 5, 2015, 12:21 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 5, 2015, 12:21 a.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Alexander Rojas <al...@mesosphere.io>.
> On May 9, 2015, 4:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.hpp, line 22
> > <https://reviews.apache.org/r/33823/diff/1/?file=949197#file949197line22>
> >
> > please avoid SCREAMING COMMENTS :)
> > (also, not sure what is the use of that comment? is it a TODO? should we have a #pragma? what?)
>
> Alexander Rojas wrote:
> 1. It is a warning.
> 2. The purpose of this header is to be a reflection of `include/mesos/scheduler/scheduler.hpp`, and so they both have the same comments, in fact they look mostly identical which follows the maxim of being consistent.
>
> Marco Massenzio wrote:
> Well, two wrongs don't make one right.
> Maybe fix both, or at least let's not perpetuate something that maybe was not ideal to start with.
>
> Please change to something that reads like:
>
> // NOTE: this header only becomes valid after running protoc
> // and generating the equivalent .ph.h files
> // See: src/messages/mesos.proto
> (or whatever the proto file is)
>
> or something to that effect.
> SCREAMING COMMENTS ARE BAD FORM !!!!!!
> :-)
>
> (you could almost hear me screaming, could you? :)))
I think that is the point, when someone is going to fall off a cliff you don't politely ask them to be careful.
> On May 9, 2015, 4:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.proto, line 80
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line80>
> >
> > `message` is becoming a truly overloaded term here: as a Protobuf message, the Message class, this (string) variable here, the (Message) `message` field below... how about `errorMessage` instead?
>
> Alexander Rojas wrote:
> It is called like that, because it is forwarded from the master and to the master. While I agree with you in principle, you know we like consistency. Why not discuss it in the HTTP API meeting?
>
> Marco Massenzio wrote:
> I don't understand the "consistency" reference. This is an `error message` and should be named as such? what is not "consistent" about that?
> When we use the same term with (semantically) different meanings, we make our code more difficult to read and understand - people have to go back and forth - especially when, as in this case, comments are very few and far between.
The consistency argument goes as follows: It is already widely use in the rest of the code so it is better to continue using it like this. In any case, I brought this issue to the team meeting and I am dropping it as discussed there.
> On May 9, 2015, 4:27 a.m., Marco Massenzio wrote:
> > include/mesos/executor/executor.proto, line 32
> > <https://reviews.apache.org/r/33823/diff/1/?file=949198#file949198line32>
> >
> > can we add also a link to the HTTP API design doc? this way for anyone looking at the Javadoc, they can follow the link and make some sense of the generated classes.
> >
> > Also, as a general comment, I'd really like all `message`s to have at least a brief description, so that the Javadoc that gets generated is meaningful to folks writing clients.
> >
> > As a general rule, we should not expect folks who write against an API to have to read the code (they may not even be able to easily access it) and, in particular those writing Java are likely to use an IDE that will present the javadoc in a pop-up/tooltip upon auto-completion.
Added comments.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review83125
-----------------------------------------------------------
On May 20, 2015, 8:41 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 20, 2015, 8:41 a.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 33823: Protocol file with the HTTP API messages
between executor and slave
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33823/#review83125
-----------------------------------------------------------
Ship it!
Only minor nits from my side, it otherwise LGTM
include/mesos/executor/executor.hpp
<https://reviews.apache.org/r/33823/#comment133994>
please avoid SCREAMING COMMENTS :)
(also, not sure what is the use of that comment? is it a TODO? should we have a #pragma? what?)
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133992>
can we add also a link to the HTTP API design doc? this way for anyone looking at the Javadoc, they can follow the link and make some sense of the generated classes.
Also, as a general comment, I'd really like all `message`s to have at least a brief description, so that the Javadoc that gets generated is meaningful to folks writing clients.
As a general rule, we should not expect folks who write against an API to have to read the code (they may not even be able to easily access it) and, in particular those writing Java are likely to use an IDE that will present the javadoc in a pop-up/tooltip upon auto-completion.
include/mesos/executor/executor.proto
<https://reviews.apache.org/r/33823/#comment133993>
`message` is becoming a truly overloaded term here: as a Protobuf message, the Message class, this (string) variable here, the (Message) `message` field below... how about `errorMessage` instead?
- Marco Massenzio
On May 4, 2015, 10:21 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33823/
> -----------------------------------------------------------
>
> (Updated May 4, 2015, 10:21 p.m.)
>
>
> Review request for mesos, Isabel Jimenez, Marco Massenzio, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> include/mesos/executor/executor.hpp PRE-CREATION
> include/mesos/executor/executor.proto PRE-CREATION
> src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b
>
> Diff: https://reviews.apache.org/r/33823/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>