> On July 18, 2016, 12:15 a.m., Stephan Erb wrote:
> > Should we consider adopting `protobuf-java-util`'s JsonFormatter rather 
> > than implementing this on our own? 
> > 
> > * 
> > https://github.com/google/protobuf/blob/master/java/util/src/main/java/com/google/protobuf/util/JsonFormat.java
> > * 
> > http://mvnrepository.com/artifact/com.google.protobuf/protobuf-java-util/3.0.0-beta-3
> 
> Mehrdad Nurolahzade wrote:
>     Looking into protobuf util ...
> 
> Mehrdad Nurolahzade wrote:
>     I can see that we already have 
> [jackson-datatype-protobuf](https://github.com/HubSpot/jackson-datatype-protobuf)
>  library in the project that can be used to create a JSON serializer from 
> protobuf. We can probably get this task done using it. However, 
> ```protobuf-java-util``` seems to have a larger and more active community.
> 
> Mehrdad Nurolahzade wrote:
>     I spent some time trying to get ```protobuf-java-util``` to work. 
> However, it seems to a very light-weight utility designed to creat one-to-one 
> mapping from protobuf to JSON. Controls to exclude or give special treatment 
> to fields in the mapping are non-existant which makes the resulting generated 
> JSON look significantly different from what we are serving today.
>     
>     Then I looked into ```jackson-datatype-protobuf``` and I found a great 
> degree of flexibility in it to control generated JSON that I did not see in 
> ```protobuf-java-util```. To generate JSON output:
>     
>     ```
>       @GET
>       @Produces(MediaType.APPLICATION_JSON)
>       public Response getOffers() throws JsonProcessingException {
>         ObjectMapper mapper = new ObjectMapper()
>             .registerModule(new ProtobufModule())
>             
> .setPropertyNamingStrategy(PropertyNamingStrategy.CAMEL_CASE_TO_LOWER_CASE_WITH_UNDERSCORES)
>             .setSerializationInclusion(JsonInclude.Include.NON_NULL);
>         return 
> Response.ok(mapper.writeValueAsString(offerManager.getOffers())).build();
>       }
>     ```
>     
>     However, the resulting JSON output is different from what we used to 
> serve (see the attachments). If this change is acceptable (and won't break 
> anything else) I can use it as-is. Otherwise, I have to write code to 
> customize the output (filter out certain fields, change rendering, etc.) 
> which I feel somehow defeats the purpose of this refactoring.

Thanks for investigating here! 

I like the new code. Personally, I feel like this can be considered 
non-breaking change because it is kind of a debug endpoint and not really part 
of the public and stable API meant to be used by clients. To be on the safe 
side, could you please send a mail to the dev list and ask if anyone out there 
relies on it?


- Stephan


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


On July 19, 2016, 4:12 a.m., Mehrdad Nurolahzade wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> -----------------------------------------------------------
> 
> (Updated July 19, 2016, 4:12 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1736
>     https://issues.apache.org/jira/browse/AURORA-1736
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> AURORA-1736 Display reservations and persistent volumes in /offers debug http 
> endpoint
> 
> 
> Diffs
> -----
> 
>   config/legacy_untested_classes.txt 1ea2183ab20cc5c6bca147bcea4e5c708d576b62 
>   src/main/java/org/apache/aurora/scheduler/http/Offers.java 
> 80f082410896a50d86c7886736caf79581f5051c 
>   src/test/java/org/apache/aurora/scheduler/http/OffersTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/50052/diff/
> 
> 
> Testing
> -------
> 
> Manual, Jenkins, and end_to_end
> 
> 
> File Attachments
> ----------------
> 
> CURRENT
>   
> https://reviews.apache.org/media/uploaded/files/2016/07/18/1de4c357-c932-4c84-962f-4209a5b679bc__offers-old.json
> NEW
>   
> https://reviews.apache.org/media/uploaded/files/2016/07/19/799bcd1f-f9c8-4b6e-bbaa-ce8022b1dac1__offers-new.json
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Reply via email to