You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/04/11 16:35:45 UTC
Review Request 46027: Documented `libprocess` helper function.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Repository: mesos
Description
-------
This is a bit confusing, because different libprocess HTTP
clients set different headers.
Diffs
-----
3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
Diff: https://reviews.apache.org/r/46027/diff/
Testing
-------
Thanks,
Neil Conway
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Benjamin Mahler <bm...@apache.org>.
It's not a hard requirement anymore, I'm asking to maintain consistency. We
originally had the 70 comment character limit to improve readability but it
was relaxed because clang-format wasn't able to distinguish line length
between code and comments. In retrospect, relaxing this has led to some
unfortunate inconsistencies in the code ( see
http://markmail.org/message/ptwqrehbpff4eefa ). l think there's value in
wrapping to reduce jaggedness and wrapping at 70, so ideally we can get
back to 70 if there's support in clang-format. For now I would be in favor
of adding back the style guidelines, even though the style checker can't
catch these: http://markmail.org/message/ptwqrehbpff4eefa
On Wed, Apr 13, 2016 at 2:40 PM, Neil Conway <ne...@gmail.com> wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
>
> On April 13th, 2016, 9:33 p.m. UTC, *Ben Mahler* wrote:
>
> 3rdparty/libprocess/src/process.cpp
> <https://reviews.apache.org/r/46027/diff/2/?file=1340142#file1340142line559> (Diff
> revision 2)
>
> static void transport(Message* message, ProcessBase* sender = NULL)
>
> 559
>
> // Returns true if `request` contains an inbound libprocess message. A
>
> s/. A/. A/
>
> Thanks for clarifying, sorry for the confusion!
>
> On April 13th, 2016, 9:37 p.m. UTC, *Ben Mahler* wrote:
>
> Also, would you mind wrapping comments at 70 to stay consistent with most of our existing code?
>
> // Returns true if `request` contains an inbound libprocess message.// A libprocess message can either be sent by another instance of// libprocess (i.e. both of the "User-Agent" and "Libprocess-From"// headers will be set), or a client that speaks the libprocess// protocol (i.e. only the "Libprocess-From" header will be set).// This function returns true for either case.
>
> Per commit f9c2604ea97b91f8a9ec3b2863317761679b1c86, I thought that wasn't a requirement anymore?
>
>
> - Neil
>
> On April 12th, 2016, 12:23 a.m. UTC, Neil Conway wrote:
> Review request for mesos and Ben Mahler.
> By Neil Conway.
>
> *Updated April 12, 2016, 12:23 a.m.*
> *Repository: * mesos
> Description
>
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
>
> Diffs
>
> - 3rdparty/libprocess/src/process.cpp
> (5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a)
>
> View Diff <https://reviews.apache.org/r/46027/diff/>
>
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Neil Conway <ne...@gmail.com>.
> On April 13, 2016, 9:33 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, line 559
> > <https://reviews.apache.org/r/46027/diff/2/?file=1340142#file1340142line559>
> >
> > s/. A/. A/
> >
> > Thanks for clarifying, sorry for the confusion!
>
> Ben Mahler wrote:
> Also, would you mind wrapping comments at 70 to stay consistent with most of our existing code?
>
> ```
> // Returns true if `request` contains an inbound libprocess message.
> // A libprocess message can either be sent by another instance of
> // libprocess (i.e. both of the "User-Agent" and "Libprocess-From"
> // headers will be set), or a client that speaks the libprocess
> // protocol (i.e. only the "Libprocess-From" header will be set).
> // This function returns true for either case.
> ```
Per commit f9c2604ea97b91f8a9ec3b2863317761679b1c86, I thought that wasn't a requirement anymore?
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/#review128749
-----------------------------------------------------------
On April 12, 2016, 12:23 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 12:23 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
>
> Diff: https://reviews.apache.org/r/46027/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Ben Mahler <be...@gmail.com>.
> On April 13, 2016, 9:33 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, line 559
> > <https://reviews.apache.org/r/46027/diff/2/?file=1340142#file1340142line559>
> >
> > s/. A/. A/
> >
> > Thanks for clarifying, sorry for the confusion!
Also, would you mind wrapping comments at 70 to stay consistent with most of our existing code?
```
// Returns true if `request` contains an inbound libprocess message.
// A libprocess message can either be sent by another instance of
// libprocess (i.e. both of the "User-Agent" and "Libprocess-From"
// headers will be set), or a client that speaks the libprocess
// protocol (i.e. only the "Libprocess-From" header will be set).
// This function returns true for either case.
```
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/#review128749
-----------------------------------------------------------
On April 12, 2016, 12:23 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 12:23 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
>
> Diff: https://reviews.apache.org/r/46027/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/#review128749
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/process.cpp (line 559)
<https://reviews.apache.org/r/46027/#comment192211>
s/. A/. A/
Thanks for clarifying, sorry for the confusion!
- Ben Mahler
On April 12, 2016, 12:23 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> -----------------------------------------------------------
>
> (Updated April 12, 2016, 12:23 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
>
> Diff: https://reviews.apache.org/r/46027/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/
-----------------------------------------------------------
(Updated April 12, 2016, 12:23 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Clarify comment.
Repository: mesos
Description
-------
This is a bit confusing, because different libprocess HTTP
clients set different headers.
Diffs (updated)
-----
3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
Diff: https://reviews.apache.org/r/46027/diff/
Testing
-------
Thanks,
Neil Conway
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Neil Conway <ne...@gmail.com>.
> On April 11, 2016, 9:55 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 559-562
> > <https://reviews.apache.org/r/46027/diff/1/?file=1339342#file1339342line559>
> >
> > Hm.. this may be a bit confusing for those without context. A libprocess message can be sent by either libprocess itself, or by clients that speak libprocess. Here the distinction is that this will tell you if this is a libprocess message *and* it is sent by a libprocess instance.
I believe the function tells you OR, not AND -- i.e., it returns true if the message was sent by another libprocess instance, or by another client that speaks the libprocess protocol. Comment updated to clarify this.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/#review128266
-----------------------------------------------------------
On April 11, 2016, 2:35 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 2:35 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
>
> Diff: https://reviews.apache.org/r/46027/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 46027: Documented `libprocess` helper function.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46027/#review128266
-----------------------------------------------------------
3rdparty/libprocess/src/process.cpp (lines 559 - 562)
<https://reviews.apache.org/r/46027/#comment191685>
Hm.. this may be a bit confusing for those without context. A libprocess message can be sent by either libprocess itself, or by clients that speak libprocess. Here the distinction is that this will tell you if this is a libprocess message *and* it is sent by a libprocess instance.
- Ben Mahler
On April 11, 2016, 2:35 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 2:35 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/process.cpp 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a
>
> Diff: https://reviews.apache.org/r/46027/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Neil Conway
>
>