You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/09/08 23:59:53 UTC
Re: Review Request 61109: Used the default value when parsing an
optional enum field.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review185038
-----------------------------------------------------------
Ship it!
This looks pretty reasonable to me. It's unfortunate that this will convert all invalid enum names into the default value, but AFAICT that is unavoidable.
- James Peach
On July 25, 2017, 3:17 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 3:17 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the default value when parsing an optional enum field.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/1/
>
>
> Testing
> -------
>
> With this patch, when accessing master endpoint with an inexistent enum `xxx` in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., `UNKNOWN`) will be used.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61109: Used the default value when parsing an
optional enum field.
Posted by Qian Zhang <zh...@gmail.com>.
> On Sept. 9, 2017, 7:59 a.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert all invalid enum names into the default value, but AFAICT that is unavoidable.
>
> Benjamin Mahler wrote:
> Since we're talking about optional enums, it's not obvious to me whether it's better to leave it unset or to set it to the default. With a required enum, we can't leave it unset so it seems like the default value makes the most sense. However, shouldn't the caller specify the behavior they want? Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be something like `use_default_for_unknown_enum_values`?
>
> Qian Zhang wrote:
> @Ben, the problem is when `Content-Type` is `application/x-protobuf`, our current implementation is an inexistent enum value will be parsed to the default enum value (i.e., `UNKNOWN`), that is what we have done in MESOS-4997, but when `Content-Type` is `application/json`, the current behavior is different: when parsing an inexistent enum value, we will get an error like `Failed to find enum for 'xxx'` rather than parsing it to the default enum value. So in this patch, I just want to make the two protocols (`application/x-protobuf` and `application/json`) have consistent behavior.
>
> Benjamin Mahler wrote:
> I see, so this is aiming to make it consistent:
>
> (1) protobuf: unknown enum value -> set to default
> (2) json before this change: unknown enum value -> error
> (3) json after this change: unknown enum value -> set to default
>
> When you say "our implementation" for (1), are you referring to what the protobuf parsing functions are doing? Or something that we implemented? If it's the former, then this change sounds good to me, since we're just mimicking the protobuf library parsing behavior in JSON.
> When you say "our implementation" for (1), are you referring to what the protobuf parsing functions are doing? Or something that we implemented?
I am referring to the work that we have done in MESOS-4997, i.e., always use optional enum field and include an UNKNOWN value as the first entry in the enum list, that way any inexistent enum value will be parsed to the default enum value (i.e., UNKNOWN). However, I did not figure out an easy way to verify it, I just infer this behavior based on the description in MESOS-4997.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review185038
-----------------------------------------------------------
On July 25, 2017, 11:17 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 11:17 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the default value when parsing an optional enum field.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/1/
>
>
> Testing
> -------
>
> With this patch, when accessing master endpoint with an inexistent enum `xxx` in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., `UNKNOWN`) will be used.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61109: Used the default value when parsing an
optional enum field.
Posted by Qian Zhang <zh...@gmail.com>.
> On Sept. 9, 2017, 7:59 a.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert all invalid enum names into the default value, but AFAICT that is unavoidable.
>
> Benjamin Mahler wrote:
> Since we're talking about optional enums, it's not obvious to me whether it's better to leave it unset or to set it to the default. With a required enum, we can't leave it unset so it seems like the default value makes the most sense. However, shouldn't the caller specify the behavior they want? Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be something like `use_default_for_unknown_enum_values`?
@Ben, the problem is when `Content-Type` is `application/x-protobuf`, our current implementation is an inexistent enum value will be parsed to the default enum value (i.e., `UNKNOWN`), that is what we have done in MESOS-4997, but when `Content-Type` is `application/json`, the current behavior is different: when parsing an inexistent enum value, we will get an error like `Failed to find enum for 'xxx'` rather than parsing it to the default enum value. So in this patch, I just want to make the two protocols (`application/x-protobuf` and `application/json`) have consistent behavior.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review185038
-----------------------------------------------------------
On July 25, 2017, 11:17 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 11:17 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the default value when parsing an optional enum field.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/1/
>
>
> Testing
> -------
>
> With this patch, when accessing master endpoint with an inexistent enum `xxx` in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., `UNKNOWN`) will be used.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61109: Used the default value when parsing an
optional enum field.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Sept. 8, 2017, 11:59 p.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert all invalid enum names into the default value, but AFAICT that is unavoidable.
>
> Benjamin Mahler wrote:
> Since we're talking about optional enums, it's not obvious to me whether it's better to leave it unset or to set it to the default. With a required enum, we can't leave it unset so it seems like the default value makes the most sense. However, shouldn't the caller specify the behavior they want? Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be something like `use_default_for_unknown_enum_values`?
>
> Qian Zhang wrote:
> @Ben, the problem is when `Content-Type` is `application/x-protobuf`, our current implementation is an inexistent enum value will be parsed to the default enum value (i.e., `UNKNOWN`), that is what we have done in MESOS-4997, but when `Content-Type` is `application/json`, the current behavior is different: when parsing an inexistent enum value, we will get an error like `Failed to find enum for 'xxx'` rather than parsing it to the default enum value. So in this patch, I just want to make the two protocols (`application/x-protobuf` and `application/json`) have consistent behavior.
I see, so this is aiming to make it consistent:
(1) protobuf: unknown enum value -> set to default
(2) json before this change: unknown enum value -> error
(3) json after this change: unknown enum value -> set to default
When you say "our implementation" for (1), are you referring to what the protobuf parsing functions are doing? Or something that we implemented? If it's the former, then this change sounds good to me, since we're just mimicking the protobuf library parsing behavior in JSON.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review185038
-----------------------------------------------------------
On July 25, 2017, 3:17 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 3:17 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the default value when parsing an optional enum field.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/1/
>
>
> Testing
> -------
>
> With this patch, when accessing master endpoint with an inexistent enum `xxx` in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., `UNKNOWN`) will be used.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61109: Used the default value when parsing an
optional enum field from JSON.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Sept. 8, 2017, 11:59 p.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert all invalid enum names into the default value, but AFAICT that is unavoidable.
>
> Benjamin Mahler wrote:
> Since we're talking about optional enums, it's not obvious to me whether it's better to leave it unset or to set it to the default. With a required enum, we can't leave it unset so it seems like the default value makes the most sense. However, shouldn't the caller specify the behavior they want? Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be something like `use_default_for_unknown_enum_values`?
>
> Qian Zhang wrote:
> @Ben, the problem is when `Content-Type` is `application/x-protobuf`, our current implementation is an inexistent enum value will be parsed to the default enum value (i.e., `UNKNOWN`), that is what we have done in MESOS-4997, but when `Content-Type` is `application/json`, the current behavior is different: when parsing an inexistent enum value, we will get an error like `Failed to find enum for 'xxx'` rather than parsing it to the default enum value. So in this patch, I just want to make the two protocols (`application/x-protobuf` and `application/json`) have consistent behavior.
>
> Benjamin Mahler wrote:
> I see, so this is aiming to make it consistent:
>
> (1) protobuf: unknown enum value -> set to default
> (2) json before this change: unknown enum value -> error
> (3) json after this change: unknown enum value -> set to default
>
> When you say "our implementation" for (1), are you referring to what the protobuf parsing functions are doing? Or something that we implemented? If it's the former, then this change sounds good to me, since we're just mimicking the protobuf library parsing behavior in JSON.
>
> Qian Zhang wrote:
> > When you say "our implementation" for (1), are you referring to what the protobuf parsing functions are doing? Or something that we implemented?
>
> I am referring to the work that we have done in MESOS-4997, i.e., always use optional enum field and include an UNKNOWN value as the first entry in the enum list, that way any inexistent enum value will be parsed to the default enum value (i.e., UNKNOWN). However, I did not figure out an easy way to verify it, I just infer this behavior based on the description in MESOS-4997.
From MESOS-4997:
> https://developers.google.com/protocol-buffers/docs/proto#updating
> enum is compatible with int32, uint32, int64, and uint64 in terms of wire format (note that values will be truncated if they don't fit), but be aware that client code may treat them differently when the message is deserialized. Notably, unrecognized enum values are discarded when the message is deserialized, which makes the field's has.. accessor return false and its getter return the first value listed in the enum definition. However, an integer field will always preserve its value. Because of this, you need to be very careful when upgrading an integer to an enum in terms of receiving out of bounds enum values on the wire.
This seems to state that unknown enum values will be stripped, i.e. not set to the default. It will be unset and accessors will return the default.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review185038
-----------------------------------------------------------
On Sept. 21, 2017, 1:58 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2017, 1:58 a.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously in MESOS-4997, we have made any inexistent enum value will
> be parsed to the default enum value when parsing protobuf message from
> a serialized string. Now in this patch, I made parsing protobuf message
> from a JSON have the same behavior.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/2/
>
>
> Testing
> -------
>
> With this patch, when accessing master endpoint with an inexistent enum `xxx` in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., `UNKNOWN`) will be used.
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 61109: Used the default value when parsing an
optional enum field.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Sept. 8, 2017, 11:59 p.m., James Peach wrote:
> > This looks pretty reasonable to me. It's unfortunate that this will convert all invalid enum names into the default value, but AFAICT that is unavoidable.
Since we're talking about optional enums, it's not obvious to me whether it's better to leave it unset or to set it to the default. With a required enum, we can't leave it unset so it seems like the default value makes the most sense. However, shouldn't the caller specify the behavior they want? Much like `JsonParseOptions.ignore_unknown_fields` is an explicit option? This would be something like `use_default_for_unknown_enum_values`?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61109/#review185038
-----------------------------------------------------------
On July 25, 2017, 3:17 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61109/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 3:17 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and James Peach.
>
>
> Bugs: MESOS-7828
> https://issues.apache.org/jira/browse/MESOS-7828
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Used the default value when parsing an optional enum field.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/protobuf.hpp 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20
>
>
> Diff: https://reviews.apache.org/r/61109/diff/1/
>
>
> Testing
> -------
>
> With this patch, when accessing master endpoint with an inexistent enum `xxx` in a JSON:
> ```
> curl -X POST -H "Content-Type: application/json" -d '{"type": "xxx"}' 127.0.0.1:5050/api/v1
> ```
> The master log will be:
> ```
> I0725 23:09:53.097790 665 http.cpp:1133] HTTP POST for /master/api/v1 from 127.0.0.1:49566 with User-Agent='curl/7.47.0'
> I0725 23:09:53.098006 665 http.cpp:669] Processing call UNKNOWN
> ```
> This proves when parsing an inexistent enum the default enum value (i.e., `UNKNOWN`) will be used.
>
>
> Thanks,
>
> Qian Zhang
>
>