I'd be fine with either failing the request or fallback to null. But fallback to 0 sounds surprising and against intuition to me.
Best, Xintong On Wed, Jul 26, 2023 at 7:44 PM Austin Cawley-Edwards < austin.caw...@gmail.com> wrote: > We discussed this in FLINK-29863[1] as well. While the JSON standard > doesn’t specify using null, the JavaScript standard encoding function > writes these values as null[2]. I think this will be least surprising to > users, and agree with the other points in this direction. > > Best, > Austin > > [1]: https://issues.apache.org/jira/browse/FLINK-29863 > [2]: > > https://github.com/FasterXML/jackson-databind/issues/911#issuecomment-352443303 > > On Wed, Jul 26, 2023 at 07:29 Matthias Pohl <matthias.p...@aiven.io > .invalid> > wrote: > > > > > > > Then we'd break the API for users that did already apply workarounds > > > although the user hasn't done anything wrong. > > > > That argument would also work against introducing the 0 default value. If > > users have a workaround, introducing 0 might break their setup because > they > > might have used a different fallback value on their side. > > > > One other idea (which I'm not 100% convinced about): For infinite, would > it > > be better to use (-)Double.MAX_VALUE as a fallback? That would be closer > to > > the intention of the value than using 0. The problem with that is that > the > > JSON spec allows bigger/smaller values than that, I guess. But if that's > > considered reasonable (because Flink won't provide bigger values), we > only > > have to find a reasonable fallback for NaN. > > > > For me, using null still feels like the better approach. It enables the > > user to do error handling on their end (in contrast to hiding it from > > them). The goal is in the end to comply with the JSON spec, I guess. > > > > Do we specify somewhere that we always should use double in the code > > conventions [1]? I couldn't find anything related to this question. If > not, > > would it be reasonable to also include a fallback serializer for Float? > > > > And just for the record: We could add AggregatedMetric to the API listing > > of the FLIP. It uses "Double" as a field type: > > > > > $ grep --include="*java" -A1 -Hirn "@JsonProperty" . | grep -i > > > 'double\|float\|bigdecimal' | grep -o '.*\.java' | sort -u > > > > > > > > > ./flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/StatsSummaryDto.java > > > > > > > > > ./flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/AggregatedMetric.java > > > > > > > > > ./flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java > > > > > > > > > ./flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/JobVertexBackPressureInfo.java > > > > > > > > > ./flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/ResourceProfileInfo.java > > > > > > [1] > > > https://flink.apache.org/how-to-contribute/code-style-and-quality-common/ > > > > On Tue, Jul 25, 2023 at 10:33 AM Chesnay Schepler <ches...@apache.org> > > wrote: > > > > > Then we'd break the API for users that did already apply workarounds > > > although the user hasn't done anything wrong. > > > > > > On 25/07/2023 04:31, Xintong Song wrote: > > > >> we should treat these cases as errors > > > >> > > > > Looking at the fields listed in the FLIP, I'd agree with this > argument. > > > And > > > > based on this, shouldn't we fail the request with e.g., a status code > > > 500, > > > > rather than responding with a fallback value silently? > > > > > > > > Best, > > > > > > > > Xintong > > > > > > > > > > > > > > > > On Tue, Jul 25, 2023 at 12:22 AM Jing Ge <j...@ververica.com.invalid > > > > > wrote: > > > > > > > >> We might consider using 0 as null for values that never return 0. > But > > > null > > > >> is still not equal to 0 and it will be very difficult to let every > > > >> contributor in this community follow this rule, especially for > future > > > >> unknown APIs, which means there will be some cases that still need > > null. > > > >> Personally, I would choose accuracy over convenience and consistency > > > over > > > >> convenience. Therefore, null is recommended. > > > >> > > > >> Best regards, > > > >> Jing > > > >> > > > >> On Mon, Jul 24, 2023 at 11:48 PM Chesnay Schepler < > ches...@apache.org > > > > > > >> wrote: > > > >> > > > >>> The downside to null is that it again forces users to handle this > > case > > > >>> themselves. > > > >>> > > > >>> The reality is that there is no good default value. > > > >>> > > > >>> Ideally we fix all cases where we return such values, such that the > > > >>> fallback to 0 isn't even used. > > > >>> Arguably the same could be said for null, but I'd think that 0 is > > less > > > >>> of a surprise. > > > >>> > > > >>> On 24/07/2023 17:21, Gyula Fóra wrote: > > > >>>> I agree that it's a bit strange to have 0 as a fallback value > > because > > > >> it > > > >>>> can also naturally occur for many metrics. > > > >>>> If we want to omit the value null would be probably better as > > Matthias > > > >>>> suggested. > > > >>>> > > > >>>> Gyula > > > >>>> > > > >>>> On Mon, Jul 24, 2023 at 4:02 PM Matthias Pohl > > > >>>> <matthias.p...@aiven.io.invalid> wrote: > > > >>>> > > > >>>>> What was the reason you decided to go for 0 as the fallback value > > > >>> instead > > > >>>>> of null? Wouldn't that be a more reasonable value for error > cases? > > > >>>>> > > > >>>>> On Mon, Jul 24, 2023 at 12:51 PM Chesnay Schepler < > > > ches...@apache.org > > > >>>>> wrote: > > > >>>>> > > > >>>>>> There are a number of cases where the REST API can return > infinity > > > or > > > >>>>>> NaN for certain double fields. > > > >>>>>> > > > >>>>>> This is problematic because the JSON spec does not allow such > > > values, > > > >>>>>> and tooling working against that spec may run into issues when > > > >>>>>> encountering such a value. > > > >>>>>> > > > >>>>>> Specifically we've seen this become an issue in clients > generated > > > >> from > > > >>>>>> the OpenAPI spec. > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >>>>>> > > > >> > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263425797 > > > >>> > > > > > > > > >
