mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 59987: Added protobuf map support.
Date Thu, 01 Mar 2018 04:10:51 GMT

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



Chun and I went over this together, so feel free to reach out to either of us for discussion!
Just some suggestions to clean up the code below.

In the description, could we mention that this is for stout's json <-> protobuf conversion?
E.g.

```
Added protobuf map support to stout JSON<->protobuf conversion.

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:
  
  https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.
```


3rdparty/stout/include/stout/protobuf.hpp
Lines 32 (patched)
<https://reviews.apache.org/r/59987/#comment278571>

    FWICT, we're just supposed to include message.h for reflection:
    
    https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#Reflection
    
    And this reflection.h header seems internal and already included by message.h? It only
seems to define some pieces of the reflection code:
    
    https://github.com/google/protobuf/blob/v3.5.0/src/google/protobuf/reflection.h



3rdparty/stout/include/stout/protobuf.hpp
Lines 388 (patched)
<https://reviews.apache.org/r/59987/#comment278572>

    s/name/key/?



3rdparty/stout/include/stout/protobuf.hpp
Lines 391-487 (patched)
<https://reviews.apache.org/r/59987/#comment278577>

    It looks like we can avoid this logic since protobuf's JSON conversion allows the protobuf
key types (bool, integers, strings) to be converted from JSON strings. This means we could
just recurse for both key and value here:
    
    ```
                // Some comment explaining what map is equivalent to with
                // a reference to the google documentation.
                google::protobuf::Message* entry =
                  reflection->AddMessage(message, field);
    
                const google::protobuf::FieldDescriptor* key_field =
                  entry->GetDescriptor()->FindFieldByNumber(1);
                  
                // Maybe passing just 'key' works due to implicit construction?
                // TODO(...): This copies the key, consider avoiding this.
                Try<Nothing> apply =
                  boost::apply_visitor(Parser(entry, key_field), JSON::String(key));
    
                if (apply.isError()) {
                  return Error(apply.error());
                }
                  
                const google::protobuf::FieldDescriptor* value_field =
                  entry->GetDescriptor()->FindFieldByNumber(2);
                  
                Try<Nothing> apply =
                  boost::apply_visitor(Parser(entry, value_field), value);
    
                if (apply.isError()) {
                  return Error(apply.error());
                }
    ```
    
    Now, to make this simplification work, we need to update our JSON conversion in a separate
patch to allow bools and integers to be parsed from JSON strings to match google's logic for
conversion:
    
    https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
    
    Documentation here: https://developers.google.com/protocol-buffers/docs/proto3#json



3rdparty/stout/include/stout/protobuf.hpp
Lines 394-398 (patched)
<https://reviews.apache.org/r/59987/#comment278573>

    Can we reference the documentation here and mention that a map is equivalent to:
    
    ```
    message MapFieldEntry {
      optional key_type key = 1;
      optional value_type value = 2;
    }
    
    repeated MapFieldEntry map_field = N;
    ```



3rdparty/stout/include/stout/protobuf.hpp
Lines 400-401 (patched)
<https://reviews.apache.org/r/59987/#comment278578>

    Can you move this out of the loop?



3rdparty/stout/include/stout/protobuf.hpp
Lines 1036-1037 (patched)
<https://reviews.apache.org/r/59987/#comment278579>

    Google converts 64 bit integers into strings, it's pretty badly broken right now that
we don't do this. I think we're going to have to switch to string and just deal with breaking
any clients that assumed strings were not coming out. I don't know if you want to help fix
this, but if you do I would be happy to review / discuss!



3rdparty/stout/include/stout/protobuf.hpp
Lines 1039-1043 (patched)
<https://reviews.apache.org/r/59987/#comment278580>

    Is there a way to abstract out this logic into a lambda here and re-use it? E.g.
    
    ```
    template <typename T>
    JSON::Value value_for_field(
        Message* entry,
        google::protobuf::Reflection* reflection,
        google::protobuf::FieldDescriptor* field)
    {
      switch (type) {
        case ...:
        case TYPE_INT64:
          return ...;
      }
    };
    ```
    
    Then you just do the following here and elsewhere:
    
    ```
    map.values[key] = value_for_field(entry, reflection, field);
    ```
    
    I've glossed over some details, but I hope you get the idea and can figure out how to
make it work across the different cases here.


- Benjamin Mahler


On Feb. 28, 2018, 11:27 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2018, 11:27 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and Zhitao
Li.
> 
> 
> Bugs: MESOS-7656
>     https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -----
> 
>   3rdparty/stout/include/stout/protobuf.hpp 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a

> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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