> On Dec. 16, 2015, 4:12 p.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.
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? - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41188/#review110804 ----------------------------------------------------------- On Dec. 16, 2015, 7:10 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41188/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2015, 7:10 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 > >
