> On Sept. 29, 2015, 3:48 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, line 1844
> > <https://reviews.apache.org/r/38335/diff/3/?file=1084054#file1084054line1844>
> >
> >     We tend not to use `typedef`s in the codebase. However, this looks like 
> > a good idea, but I would place it in `Resources`, e.g. 
> > `Resources::ProtoType`.
> >     
> >     However, I'll leave the decision whether to use `typedef` or not to you 
> > and your shepherd.
> 
> Klaus Ma wrote:
>     It's related to test only, so keep it in test module for now.

This doesn't seem like an intuitive typedef nor is it all that helpful. I think 
it would be better to use a `using` declaration instead.

`using google::protobuf::RepeatedPtrField;` then we can use 
`RepeatedPtrField<Resource>`. I would also suggest breaking it out of the 
single expression to reduce the clutterness. For example:

```cpp
string body = "resources=" +
              stringify(JSON::protobuf(
                  static_cast<ResourcesProtobufType>(dynamicallyReserved)));
```
could be:
```cpp
auto protobuf = JSON::protobuf(
    static_cast<const RepeatedPtrField<Resource>&>(dynamicallyReserved));
    
string body = "resources=" + stringify(protobuf);
```

Sidenote: the `static_cast` to `const &` is to avoid an unnecessary copy.


- Michael


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


On Oct. 20, 2015, 5:37 a.m., Klaus Ma wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38335/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2015, 5:37 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
> -----
> 
>   src/common/http.cpp f56d8a1 
>   src/docker/executor.cpp 1e49013 
>   src/examples/persistent_volume_framework.cpp 176ac3d 
>   src/launcher/executor.cpp 50b3c6e 
>   src/master/contender.cpp c641305 
>   src/master/http.cpp 093f793 
>   src/master/maintenance.cpp 5fe9358 
>   src/master/registrar.cpp 1117232 
>   src/slave/containerizer/fetcher.cpp e0d02d5 
>   src/slave/containerizer/mesos/containerizer.cpp d1fc5a4 
>   src/slave/monitor.cpp aa6e958 
>   src/tests/containerizer/launch_tests.cpp de655ec 
>   src/tests/fault_tolerance_tests.cpp f78a291 
>   src/tests/master_maintenance_tests.cpp e89ce3b 
>   src/tests/master_tests.cpp ee24739 
>   src/tests/mesos.hpp 3e58b45 
>   src/tests/mesos.cpp ab2d85b 
>   src/tests/monitor_tests.cpp 583e711 
>   src/tests/reservation_endpoints_tests.cpp f5f9c48 
>   src/tests/resources_tests.cpp 6584fc6 
>   src/tests/scheduler_http_api_tests.cpp d338a1b 
>   src/tests/script.cpp bcc1fab 
>   src/tests/slave_tests.cpp 10a4fa7 
>   src/usage/main.cpp 86fd796 
> 
> Diff: https://reviews.apache.org/r/38335/diff/
> 
> 
> Testing
> -------
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Reply via email to