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