> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > <https://reviews.apache.org/r/41188/diff/6/?file=1165163#file1165163line130>
> >
> >     Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> >     
> >     AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.
> 
> Avinash sridharan wrote:
>     I am trying to understand this comment. Is it that applications don't 
> expect nested labels (for ports) when they read state.json? 
>     
>     In terms of exposing labels in state.json I can see from 
> TEST_F(SlaveTest, TaskLabels) (slave_tests.cpp) the labels are nested within 
> tasks. Since, DiscoveryInfo also has labels and Port has labels unless we 
> nest them, the relationship would not be explicit if we do not replicate the 
> protobuf right?
> 
> Anand Mazumdar wrote:
>     Let me give you an example:
>     
>     Here is how the Task Labels JSON returned from this test looks like:
>     
>     ```
>     "tasks": [
>                 {
>                   "executor_id": "default",
>                   "framework_id": "5452180f-fa83-4d25-802f-8a0c974dfd21-0000",
>                   "id": "1",
>                   "labels": [
>                     {
>                       "key": "foo",
>                       "value": "bar"
>                     },
>                     {
>                       "key": "bar",
>                       "value": "baz"
>                     },
>                     {
>                       "key": "bar",
>                       "value": "qux"
>                     }
>                   ],
>                   "name": "",
>                   "resources": {
>                     "cpus": 2,
>                     "disk": 1024,
>                     "mem": 1024,
>                     "ports": "[31000-32000]"
>                   },
>     ```
>     
>     If you notice, it does not have a "labels" field nested inside a "labels" 
> field as we have when we do automated protobuf to JSON conversions. Do we 
> want to preserve this behavior ?
> 
> Avinash sridharan wrote:
>     I get your point . You are right this is an artifact of using the 
> JSON::protobuf compred to explicit model functions . 
>     
>     Is this going to be an issue? Since the dependency is reflected in the 
> protobuf definition.
>     One reason I can of think of keeping it as is, is that the protobuf is a 
> contract between the framework and service discovery, so they might actually 
> expect this.
> 
> Adam B wrote:
>     Note that your proposed changes also have "ports" nested under "ports".
>     Arguments for the compact (no nesting) format:
>     + App: Shorter find/get query strings, more compact json output to read.
>     + Perf: Less json to generate, fewer bytes over the wire.
>     + Not matching exact protobuf format means we can break the format easier 
> later.
>     Arguments for nested format:
>     + Direct json translation to/from protobuf; otherwise, all 
> clients/readers will have to manually update their parsing (in a potentially 
> undocumented manner) anytime the data is modeled differently.
>     
>     Also note that this nested protobuf pattern is one of the less fortunate 
> side effects of having an `optional Labels labels` field instead of directly 
> using a `repeated Label labels` field. Maybe we should switch to the 
> `repeated` pattern for all labels/ports so we can use the direct 
> json/protobuf translation functions for everything?

Based on discussion with Adam we are going to keep it simple (though verbose) 
for now. With Mpark's changes in the near future all this would change/improve 
so tabling this discussion and sticking to the simple by keeping everything as 
is (in this code).


- Avinash


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


On Dec. 17, 2015, 7:51 p.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2015, 7:51 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
>     https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> -------
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Reply via email to