mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Avinash sridharan" <avin...@mesosphere.io>
Subject Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints
Date Thu, 17 Dec 2015 04:33:50 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message