Re: 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/ --- (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
--- 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
--- 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
--- 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
--- 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
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
--- 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
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
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
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
--- 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
--- 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
