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

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.


- Avinash


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


On Dec. 17, 2015, 3:10 a.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2015, 3:10 a.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