Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Adam B]

> On Feb. 18, 2016, 10:32 p.m., Adam B wrote:
> > include/mesos/mesos.proto, line 1600
> > 
> >
> > Is it ok for labels to contain duplicate keys even if the values are 
> > different?
> > That sounds like undefined behavior too. Is the label-consumer supposed 
> > to use all values, first value, last value?
> 
> Neil Conway wrote:
> To me, the issue isn't how the label-consumer is supposed to interpret 
> the labels: rather, labels with duplicate key-value pairs are not handled 
> correctly by Mesos (our equality operator is wrong for this situation -- see 
> MESOS-4445). The initial feeling was that the runtime cost of fixing the 
> equality operator wasn't worth it (although based on the experiments in 
> https://reviews.apache.org/r/43686/, it is unclear whether this is true).
> 
> The equality operator behaves correctly for labels with duplicate keys 
> but distinct values associated with those keys. How label consumers are 
> supposed to interpret them is up to the application.

I see. You're working on preventing incorrect behavior first, rather than 
worrying about preventing (or defining) undefined behavior. Give them a shorter 
noose. Works for me. We can clarify semantics for duplicate keys later.


> On Feb. 18, 2016, 10:32 p.m., Adam B wrote:
> > include/mesos/mesos.proto, line 1634
> > 
> >
> > Are you intentionally leaving out the Labels field in DiscoveryInfo, 
> > Port, NetworkInfo, Image.Appc? I didn't think Mesos interpreted any of 
> > these either.
> 
> Neil Conway wrote:
> It just seemed a bit verbose to copy the same comment that many times, so 
> I settled for documenting the most common use-sites as well as the definition 
> of `Labels`. Happy to change that if people would rather see it done another 
> way though.

Yeah, I agree that the only necessary one is the root Labels message, and the 
other comments are bonus. Just wanted to make sure it was a conscious choice. 
Dropping the issue.


- Adam


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


On Feb. 17, 2016, 10:44 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 17, 2016, 10:44 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Neil Conway]

> On Feb. 19, 2016, 6:32 a.m., Adam B wrote:
> > include/mesos/mesos.proto, line 1600
> > 
> >
> > Is it ok for labels to contain duplicate keys even if the values are 
> > different?
> > That sounds like undefined behavior too. Is the label-consumer supposed 
> > to use all values, first value, last value?

To me, the issue isn't how the label-consumer is supposed to interpret the 
labels: rather, labels with duplicate key-value pairs are not handled correctly 
by Mesos (our equality operator is wrong for this situation -- see MESOS-4445). 
The initial feeling was that the runtime cost of fixing the equality operator 
wasn't worth it (although based on the experiments in 
https://reviews.apache.org/r/43686/, it is unclear whether this is true).

The equality operator behaves correctly for labels with duplicate keys but 
distinct values associated with those keys. How label consumers are supposed to 
interpret them is up to the application.


> On Feb. 19, 2016, 6:32 a.m., Adam B wrote:
> > include/mesos/mesos.proto, line 1634
> > 
> >
> > Are you intentionally leaving out the Labels field in DiscoveryInfo, 
> > Port, NetworkInfo, Image.Appc? I didn't think Mesos interpreted any of 
> > these either.

It just seemed a bit verbose to copy the same comment that many times, so I 
settled for documenting the most common use-sites as well as the definition of 
`Labels`. Happy to change that if people would rather see it done another way 
though.


- Neil


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


On Feb. 17, 2016, 6:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 17, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Adam B]

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



Thanks for putting this together. Just a couple of questions.


include/mesos/mesos.proto (line 1600)


Is it ok for labels to contain duplicate keys even if the values are 
different?
That sounds like undefined behavior too. Is the label-consumer supposed to 
use all values, first value, last value?



include/mesos/mesos.proto (line 1634)


Are you intentionally leaving out the Labels field in DiscoveryInfo, Port, 
NetworkInfo, Image.Appc? I didn't think Mesos interpreted any of these either.


- Adam B


On Feb. 17, 2016, 10:44 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 17, 2016, 10:44 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Guangya Liu]

> On 二月 17, 2016, 1:57 a.m., Guangya Liu wrote:
> > include/mesos/mesos.proto, line 598
> > 
> >
> > s/Labels/One label?
> > 
> > I think that here we should use `one label` but not `labels`, becauase 
> > this can only happen if one label include duplicate key-value pairs.
> 
> Neil Conway wrote:
> Among all the key-value pairs in a `Labels`, no two pairs should be the 
> same. So I think the current text expresses that better than talking about an 
> individual label.
> 
> Guangya Liu wrote:
> May be we need to be more accurate, if `Labels should not contain 
> duplicate key-value pairs`, then how can mesos define same labels?
> 
> Neil Conway wrote:
> "Among all the key-value pairs in a Labels, no two pairs should be the 
> same." -- i.e., "[foo -> bar, foo -> bar]" is not supported. "[foo -> bar, 
> foo -> baz]" will work (although debatable whether it is a good idea).

Yes, I mean the following case: I want to define two same labels, label1 
[foo->bar], label2 [foo->bar], those are two same labels, but i think that 
current description is good enough, thanks.


- Guangya


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


On 二月 17, 2016, 6:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated 二月 17, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Neil Conway]

> On Feb. 17, 2016, 1:57 a.m., Guangya Liu wrote:
> > include/mesos/mesos.proto, line 598
> > 
> >
> > s/Labels/One label?
> > 
> > I think that here we should use `one label` but not `labels`, becauase 
> > this can only happen if one label include duplicate key-value pairs.
> 
> Neil Conway wrote:
> Among all the key-value pairs in a `Labels`, no two pairs should be the 
> same. So I think the current text expresses that better than talking about an 
> individual label.
> 
> Guangya Liu wrote:
> May be we need to be more accurate, if `Labels should not contain 
> duplicate key-value pairs`, then how can mesos define same labels?

"Among all the key-value pairs in a Labels, no two pairs should be the same." 
-- i.e., "[foo -> bar, foo -> bar]" is not supported. "[foo -> bar, foo -> 
baz]" will work (although debatable whether it is a good idea).


- Neil


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


On Feb. 17, 2016, 6:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 17, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Guangya Liu]

> On 二月 17, 2016, 1:57 a.m., Guangya Liu wrote:
> > include/mesos/mesos.proto, line 598
> > 
> >
> > s/Labels/One label?
> > 
> > I think that here we should use `one label` but not `labels`, becauase 
> > this can only happen if one label include duplicate key-value pairs.
> 
> Neil Conway wrote:
> Among all the key-value pairs in a `Labels`, no two pairs should be the 
> same. So I think the current text expresses that better than talking about an 
> individual label.

May be we need to be more accurate, if `Labels should not contain duplicate 
key-value pairs`, then how can mesos define same labels?


- Guangya


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


On 二月 17, 2016, 6:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated 二月 17, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43616]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 17, 2016, 6:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 17, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e24d3e03a7dc7c6bfd07f34531cb593fe4925646 
>   include/mesos/v1/mesos.proto d909e60ddfd8e3ba2075f82c372edde04cd99d54 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Neil Conway]

> On Feb. 17, 2016, 1:57 a.m., Guangya Liu wrote:
> > include/mesos/mesos.proto, line 598
> > 
> >
> > s/Labels/One label?
> > 
> > I think that here we should use `one label` but not `labels`, becauase 
> > this can only happen if one label include duplicate key-value pairs.

Among all the key-value pairs in a `Labels`, no two pairs should be the same. 
So I think the current text expresses that better than talking about an 
individual label.


- Neil


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


On Feb. 16, 2016, 7:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 16, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0bd5abadb5abe052161963ca995c396f1ed832f2 
>   include/mesos/v1/mesos.proto 38e04cb19e303d1c71d2afad6ea73137aaa7403a 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Guangya Liu]

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




include/mesos/mesos.proto (line 598)


s/Labels/One label?

I think that here we should use `one label` but not `labels`, becauase this 
can only happen if one label include duplicate key-value pairs.


- Guangya Liu


On 二月 16, 2016, 7:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated 二月 16, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0bd5abadb5abe052161963ca995c396f1ed832f2 
>   include/mesos/v1/mesos.proto 38e04cb19e303d1c71d2afad6ea73137aaa7403a 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43616: Documented that labels should not contain duplicate key-value pairs.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [43616]

Failed command: ./support/apply-review.sh -n -r 43616

Error:
2016-02-16 23:24:02 URL:https://reviews.apache.org/r/43616/diff/raw/ 
[4159/4159] -> "43616.patch" [1]
error: patch failed: include/mesos/mesos.proto:594
error: include/mesos/mesos.proto: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/11479/console

- Mesos ReviewBot


On Feb. 16, 2016, 7:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43616/
> ---
> 
> (Updated Feb. 16, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The implementation of the equality operator for `Labels` is buggy for labels
> that contain duplicates. We might want to revisit fixing the implementation of
> that operator (which might be expensive; MESOS-4445), but in the short-term we
> should document that duplicates should not be specified.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0bd5abadb5abe052161963ca995c396f1ed832f2 
>   include/mesos/v1/mesos.proto 38e04cb19e303d1c71d2afad6ea73137aaa7403a 
> 
> Diff: https://reviews.apache.org/r/43616/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>