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