mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)
Date Tue, 29 Sep 2015 12:14:48 GMT

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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 621)
<https://reviews.apache.org/r/38342/#comment158240>

    Not yours, but while you're editing here, can you `.reserve()` please?



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 638)
<https://reviews.apache.org/r/38342/#comment158241>

    ditto



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 760 - 761)
<https://reviews.apache.org/r/38342/#comment158239>

    I've seen your discussion with @Jan above, here is what I think regarding `ABORT` vs.
`Try<>`. 
    
    First off, some background. We tend to use `ABORT()` and `CHECK_*` macros when so-called
"internal invariant" is violated, for example an object is being used without proper initialization,
or a change is being made to an instance, that does not exists any more. On the other side
`Try<>` and `Error<>` family is used when our expectation about the outer world
does not hold, or, in other words, when an action cannot be completed, but no internal invariant
is broken. User input, I/O, network operations are good examples of the latter case.
    
    Let's try to figure out what we have here. I would say, it's an internal invariant, and
here is why. JSON is less strict as Protobuf, therefore conversion Protobuf->JSON always
exists (note that this is not symmetrical, JSON->Protobuf is not always possible, hence
we use `Try<>` in e.g. `protobuf::parse<>()`). The only reason it may fail (remember
we do not handle OOM exceptions) is because we convert a protobuf message of a yet unknown
format (most probably future proto versions), which is a violation of an internal invariant!
    
    Therefore I would suggest we keep `ABORT()` but document in the preambular comment why
we use `ABORT()` and not `Try<>`, explaining what internal invariant is in this case.


- Alexander Rukletsov


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> -----------------------------------------------------------
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
>     https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which converts a
`google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField<T>` by introducing
overloaded functions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> -------
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message