You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2016/01/12 00:39:08 UTC
Review Request 42167: Fixed stout protobuf::parse passing error
message in nested JSON.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42167/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
Bugs: MESOS-4311
https://issues.apache.org/jira/browse/MESOS-4311
Repository: mesos
Description
-------
Fixed stout protobuf::parse passing error message in nested JSON.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 98ea47794b3a7c99b3cbd2418ba6e36eb5951259
Diff: https://reviews.apache.org/r/42167/diff/
Testing
-------
make check (ubuntu14.04 + clang-3.6)
Thanks,
Gilbert Song
Re: Review Request 42167: Fixed stout protobuf::parse passing error
message in nested JSON.
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 12, 2016, 1:24 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 336
> > <https://reviews.apache.org/r/42167/diff/1/?file=1192483#file1192483line336>
> >
> > s/for recursive/for the recursive/
> > s/sequencially/sequentially/
We ended up re-phrasing this one, thanks!
> On Jan. 12, 2016, 1:24 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 339-346
> > <https://reviews.apache.org/r/42167/diff/1/?file=1192483#file1192483line339>
> >
> > Wouldn't it be simpler to do this?
> > `return parse(reflection->AddMessage(message, field), object);`
> >
> > The return types are the same.
> >
> > Ditto for the `else`.
Thanks!
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42167/#review113894
-----------------------------------------------------------
On Jan. 11, 2016, 11:39 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42167/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 11:39 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
>
>
> Bugs: MESOS-4311
> https://issues.apache.org/jira/browse/MESOS-4311
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed stout protobuf::parse passing error message in nested JSON.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 98ea47794b3a7c99b3cbd2418ba6e36eb5951259
>
> Diff: https://reviews.apache.org/r/42167/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 42167: Fixed stout protobuf::parse passing error
message in nested JSON.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42167/#review113894
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 336)
<https://reviews.apache.org/r/42167/#comment174678>
s/for recursive/for the recursive/
s/sequencially/sequentially/
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 339 - 346)
<https://reviews.apache.org/r/42167/#comment174677>
Wouldn't it be simpler to do this?
`return parse(reflection->AddMessage(message, field), object);`
The return types are the same.
Ditto for the `else`.
- Joseph Wu
On Jan. 11, 2016, 3:39 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42167/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 3:39 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
>
>
> Bugs: MESOS-4311
> https://issues.apache.org/jira/browse/MESOS-4311
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed stout protobuf::parse passing error message in nested JSON.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 98ea47794b3a7c99b3cbd2418ba6e36eb5951259
>
> Diff: https://reviews.apache.org/r/42167/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 42167: Fixed stout protobuf::parse passing error
message in nested JSON.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42167/#review113887
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 335 - 337)
<https://reviews.apache.org/r/42167/#comment174669>
How about:
```
// TODO(gilbert): We currently push up the nested error
// messages without wrapping the error message (due to
// the recursive nature of parse). We should pass along
// variable information in order to construct a helpful
// error message, e.g. "Failed to parse field 'a.b.c': ...".
```
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 342 - 343)
<https://reviews.apache.org/r/42167/#comment174671>
Looks like we can just make do with the TODO above, rather than having these extra comments?
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 348)
<https://reviews.apache.org/r/42167/#comment174665>
Can we avoid naming these variables after the if condition? How about 'parse' for both of these?
- Ben Mahler
On Jan. 11, 2016, 11:39 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42167/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 11:39 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
>
>
> Bugs: MESOS-4311
> https://issues.apache.org/jira/browse/MESOS-4311
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed stout protobuf::parse passing error message in nested JSON.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 98ea47794b3a7c99b3cbd2418ba6e36eb5951259
>
> Diff: https://reviews.apache.org/r/42167/diff/
>
>
> Testing
> -------
>
> make check (ubuntu14.04 + clang-3.6)
>
>
> Thanks,
>
> Gilbert Song
>
>