Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-20 Thread Joshua Cohen 

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


Ship it!




I'm not super bothered by the double labels, so I'm ok with shipping this.

If others feel strongly that they're problematic then we can investigate a 
custom jackson serializer to clean them up.

- Joshua Cohen


On July 20, 2016, 4:26 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 20, 2016, 4:26 p.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
> -
> 
>   RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 
>   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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-20 Thread Mehrdad Nurolahzade 

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

(Updated July 20, 2016, 9:26 a.m.)


Review request for Aurora, Joshua Cohen and Stephan Erb.


Changes
---

Updated release notes


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 (updated)
-

  RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 
  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

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-19 Thread Stephan Erb 


> On July 19, 2016, 11:13 p.m., Dmitriy Shirchenko wrote:
> > File Attachment: NEW - offers-new.json
> > 
> >
> > Why is there a double nested labels: labels array under reservation? Is 
> > this just a carry over from how Aurora gets offers from Mesos?
> > 
> > So there is a key `labels` and then underneath is there's another key 
> > `labels` whose value is an array. Seems like one of them is not necessary.
> > 
> > ```json
> > reservation: {
> > labels: {
> > labels: [
> > {
> > key: "job",
> > value: "devcluster/www-data/prod/hello"
> > }
> > ]
> > }
> > }
> > ```
> 
> Mehrdad Nurolahzade wrote:
> This is a question we have to put to Mesos people, it's a weired design. 
> I can confirm that it is not due to our rendering of their protobuf 
> structures.
> 
> To verify I played with their 
> ```src/tests/reservation_endpoints_tests.cpp``` tests and dumped their 
> ```ReservationInfo``` structures and noticed the same weired nest behavior. 
> Also, if you try to set labels through supplied http endpoints ```/reserve``` 
> you would notice the call would fail unless you supply nested labels:
> ```
> curl -i \
>   -d slaveId="d091a635-33c8-4409-989f-9bef16e36f34-S0" \
>   -d resources='[
> {
>   "name": "disk",
>   "type": "SCALAR",
>   "scalar": { "value": 128 },
>   "role": "aurora-role",
>  "reservation": {
>  "labels": {
>"labels" : [{"key": "job", "value": 
> "devcluster/www-data/prod/hello"}]
>}
>  }
> }
>   ]' \
>   -X POST http://192.168.33.7:5050/master/reserve
> ```

Looks like this is due to a sub-optimal desire for code reuse: 

* Definition: 
https://github.com/apache/mesos/blob/cbfd3575414eef6f2faf249b4b0a2a8de015cc42/include/mesos/mesos.proto#L1980-L1991
 
* Usage (one of many): 
https://github.com/apache/mesos/blob/cbfd3575414eef6f2faf249b4b0a2a8de015cc42/include/mesos/mesos.proto#L692-L698

We would not see this, if the would be using `repeated Label labels` 
everywhere, rather than `optional Labels labels`.


- Stephan


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


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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-19 Thread Mehrdad Nurolahzade 


> On July 19, 2016, 2:13 p.m., Dmitriy Shirchenko wrote:
> > File Attachment: NEW - offers-new.json
> > 
> >
> > Why is there a double nested labels: labels array under reservation? Is 
> > this just a carry over from how Aurora gets offers from Mesos?
> > 
> > So there is a key `labels` and then underneath is there's another key 
> > `labels` whose value is an array. Seems like one of them is not necessary.
> > 
> > ```json
> > reservation: {
> > labels: {
> > labels: [
> > {
> > key: "job",
> > value: "devcluster/www-data/prod/hello"
> > }
> > ]
> > }
> > }
> > ```

This is a question we have to put to Mesos people, it's a weired design. I can 
confirm that it is not due to our rendering of their protobuf structures.

To verify I played with their ```src/tests/reservation_endpoints_tests.cpp``` 
tests and dumped their ```ReservationInfo``` structures and noticed the same 
weired nest behavior. Also, if you try to set labels through supplied http 
endpoints ```/reserve``` you would notice the call would fail unless you supply 
nested labels:
```
curl -i \
  -d slaveId="d091a635-33c8-4409-989f-9bef16e36f34-S0" \
  -d resources='[
{
  "name": "disk",
  "type": "SCALAR",
  "scalar": { "value": 128 },
  "role": "aurora-role",
 "reservation": {
   "labels": {
 "labels" : [{"key": "job", "value": 
"devcluster/www-data/prod/hello"}]
   }
 }
}
  ]' \
  -X POST http://192.168.33.7:5050/master/reserve
```


- Mehrdad


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


On July 18, 2016, 7:12 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 18, 2016, 7:12 p.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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-19 Thread Dmitriy Shirchenko 

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




File Attachment: NEW - offers-new.json


Why is there a double nested labels: labels array under reservation? Is 
this just a carry over from how Aurora gets offers from Mesos?

So there is a key `labels` and then underneath is there's another key 
`labels` whose value is an array. Seems like one of them is not necessary.

```json
reservation: {
labels: {
labels: [
{
key: "job",
value: "devcluster/www-data/prod/hello"
}
]
}
}
```


- Dmitriy Shirchenko


On July 19, 2016, 2: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, 2: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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-19 Thread Stephan Erb 

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


Fix it, then Ship it!




Looks good to me!

Additional things that would be neat:

* changelog entry
* mail to the dev list to confirm nobody relies on the exact json layout 
returned here


src/test/java/org/apache/aurora/scheduler/http/OffersTest.java (lines 143 - 149)


The test is very closly following implementation of the code we want to 
test. 

Even though it comes with boilerplate, I believe we gain clarity if you 
compare the request result to a plain text json that we keep as a text fixture, 
rather than parsing the json back into an offer.

I don't have a really strong opinion here, especially given that the class 
was untested before. Feel free to ignore if you see this otherwise.


- Stephan Erb


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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-19 Thread Stephan Erb 


> 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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Aurora ReviewBot 

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


Ship it!




Master (e67c6a7) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On July 19, 2016, 2: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, 2: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
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Mehrdad Nurolahzade 

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

(Updated July 18, 2016, 7:12 p.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 (updated)


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

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Mehrdad Nurolahzade 

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

(Updated July 18, 2016, 7:11 p.m.)


Review request for Aurora, Joshua Cohen and Stephan Erb.


Changes
---

Rendering JSON using ```jackson-datatype-protobuf``` library


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 (updated)
-

  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
MODIFIED
  
https://reviews.apache.org/media/uploaded/files/2016/07/18/50faae5c-45ac-45af-aa3c-d68ca09d2e72__offers-new.json


Thanks,

Mehrdad Nurolahzade

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Mehrdad Nurolahzade 

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

(Updated July 18, 2016, 12:32 p.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 (updated)


CURRENT
  
https://reviews.apache.org/media/uploaded/files/2016/07/18/1de4c357-c932-4c84-962f-4209a5b679bc__offers-old.json
MODIFIED
  
https://reviews.apache.org/media/uploaded/files/2016/07/18/50faae5c-45ac-45af-aa3c-d68ca09d2e72__offers-new.json


Thanks,

Mehrdad Nurolahzade

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Mehrdad Nurolahzade 


> On July 17, 2016, 3:15 p.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.

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.


- Mehrdad


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


On July 15, 2016, 1:15 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 15, 2016, 1:15 p.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
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Mehrdad Nurolahzade 


> On July 17, 2016, 3:15 p.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 ...

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


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


On July 15, 2016, 1:15 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 15, 2016, 1:15 p.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
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-18 Thread Mehrdad Nurolahzade 


> On July 17, 2016, 3:15 p.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

Looking into protobuf util ...


- Mehrdad


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


On July 15, 2016, 1:15 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 15, 2016, 1:15 p.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
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-17 Thread Stephan Erb 

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



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

- Stephan Erb


On July 15, 2016, 10:15 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 15, 2016, 10:15 p.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
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-15 Thread Aurora ReviewBot 

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



Master (e67c6a7) is red with this patch.
  ./build-support/jenkins/build.sh

   proxy_driver = ProxyDriver()
   with temporary_dir() as checkpoint_root:
 te = AuroraExecutor(
 >   
runner_provider=make_provider(checkpoint_root),
 
sandbox_provider=DefaultTestSandboxProvider())
 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:580: 
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:193: in 
make_provider
 pex_location=thermos_runner_path(),
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 build = True
 
 def thermos_runner_path(build=True):
   if not build:
 return getattr(thermos_runner_path, 'value', 
None)
 
   if not hasattr(thermos_runner_path, 'value'):
 pex_dir = safe_mkdtemp()
 >   assert subprocess.call(["./pants", 
"--pants-distdir=%s" % pex_dir, "binary",
   
"src/main/python/apache/thermos/runner:thermos_runner"]) == 0
 E   assert 1 == 0
 E+  where 1 = (['./pants', '--pants-distdir=/tmp/tmppmzdj3', 'binary', 
'src/main/python/apache/thermos/runner:thermos_runner'])
 E+where  = subprocess.call
 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:185: 
AssertionError
 -- Captured stderr call --
 Traceback (most recent call last):
   File 
"/home/jenkins/jenkins-slave/workspace/AuroraBot/.home/.cache/pants/setup/bootstrap-Linux-x86_64/1.1.0-rc7/bin/pants",
 line 7, in 
 from pants.bin.pants_exe import main
 ImportError: No module named pants.bin.pants_exe
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  16 failed, 652 passed, 6 skipped, 1 warnings, 8 
error in 179.72 seconds 
 
FAILURE


20:35:27 03:24   [complete]
   FAILURE


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On July 15, 2016, 8:15 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 15, 2016, 8:15 p.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
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-15 Thread Mehrdad Nurolahzade 


> On July 14, 2016, 7:34 p.m., Joshua Cohen wrote:
> > How do you feel about adding a unit test for `getOffers` that verifies all 
> > this new logic?

I feel good. :)


> On July 14, 2016, 7:34 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/Offers.java, line 89
> > 
> >
> > Is there any reason that all of these `*_TO_BEAN` functions return an 
> > Object rather than `Map` (since that's what the root `TO_BEAN` 
> > function returns)

Correct, going to refactor all.


- Mehrdad


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


On July 14, 2016, 4:08 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 14, 2016, 4:08 p.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
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/Offers.java 
> 80f082410896a50d86c7886736caf79581f5051c 
> 
> Diff: https://reviews.apache.org/r/50052/diff/
> 
> 
> Testing
> ---
> 
> Manual, Jenkins, and end_to_end
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-14 Thread Joshua Cohen 

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



How do you feel about adding a unit test for `getOffers` that verifies all this 
new logic?


src/main/java/org/apache/aurora/scheduler/http/Offers.java (line 89)


Is there any reason that all of these `*_TO_BEAN` functions return an 
Object rather than `Map` (since that's what the root `TO_BEAN` 
function returns)


- Joshua Cohen


On July 14, 2016, 11:08 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 14, 2016, 11:08 p.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
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/Offers.java 
> 80f082410896a50d86c7886736caf79581f5051c 
> 
> Diff: https://reviews.apache.org/r/50052/diff/
> 
> 
> Testing
> ---
> 
> Manual, Jenkins, and end_to_end
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint

2016-07-14 Thread Aurora ReviewBot 

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



Master (e67c6a7) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On July 14, 2016, 11:08 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
> 
> (Updated July 14, 2016, 11:08 p.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
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/Offers.java 
> 80f082410896a50d86c7886736caf79581f5051c 
> 
> Diff: https://reviews.apache.org/r/50052/diff/
> 
> 
> Testing
> ---
> 
> Manual, Jenkins, and end_to_end
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>