[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628776#comment-17628776 ] Chesnay Schepler commented on FLINK-29872: -- ??Why would we design a non backward compatible RestClient if the spec itself is backward compatible??? a) Remember that the spec came after the current rest client was created. b) As I said before, the client within Flink can inherently not be fully compatible with all Flink versions because of the job submission. That the fields aren't marked as required wasn't actually intentional and is more a limitation of the current spec. Even if that were changed though I wouldn't oppose having a second variant that is less strict, i.e., is similar to the current spec. Having fields being marked as required simplifies things on both ends; users know up front what they have to provide and what they get back, and on our side we don't need to have every handler check 20 preconditions for each request; instead this is handled generically by the rest framework. > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628684#comment-17628684 ] Gyula Fora commented on FLINK-29872: Our goal is not to generate new clients if they are already available in Flink. Best would be to simply use it as we already use many other utilities. You actually provided an excellent argument why we should make the RestClient backward compatible. As you said a simple generated client based on the latest spec would probably work. In that case I believe we should improve the RestClient so that it comes with this nice property also. Why would we design a non backward compatible RestClient if the spec itself is backward compatible? > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628605#comment-17628605 ] Chesnay Schepler commented on FLINK-29872: -- Can you expand on why you consider generating a client to be complicated? Generating one for each Flink version, sure; but generating on for the 1.15 spec and using that going forward shouldn't be hard? You'd also further decouple the operator from the actual Flink codebase (and certain modules/dependencies?), which should be a long-term goal anyway. (additionally you wouldn't have to wait for a Flink release to benefit from changes to the spec) You're probably gonna argue that you'd like to use 1.16+ features of the REST API; however you could probably even use a client generated from the 1.16 spec. Because we aren't actually marking fields as required in there a generated client _should_ treat everything as optional, exactly as you propose. I really don't get what the big deal is supposed to be. > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628537#comment-17628537 ] Gyula Fora commented on FLINK-29872: You are right we could do many complicated things that might solve a problem that is introduced by non backward compatible clients. It might not be a requirement at the moment that the Rest Client is backward compatible but maybe it should be. We have to understand the costs and the benefits. We can't simply look at the costs. I understand that you have a different personal opinion, in that case the easiest is to discuss in the community because I disagree. If we decide to add backward compatibilty we might need some null checks or mechanism to set defaults , but what we get in return can be very valuable for certain users. > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628530#comment-17628530 ] Chesnay Schepler commented on FLINK-29872: -- Then frankly you should use the client from the oldest supported version, generate a client from the OpenAPI spec for the oldest supported version, or generate different clients for each Flink version from the OpenAPI spec. ??We simply should not introduce required fields in the rest api.?? Of course we should. Treating every single response field as nullable from now until the end of time is terrible. I'm not looking forward to having to null-check every little thing. > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628516#comment-17628516 ] Gyula Fora commented on FLINK-29872: With components such as the Kubernetes Operator these compatibilty questions become more important. The current operator needs to talk to multiple Flink versions at the same time to perform basic cluster operations (ideally using the latest supported Flink client) It is trivial to fix these incompatibilities on Flink rest side and that allows us to use the operator without having to add copies of fixed classes to shadow what comes from flink. We simply should not introduce required fields in the rest api. It's easy not to do it with some attention and I don't see a reason why we would anyway. > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (FLINK-29872) JobDetailsInfo Rest endpoint breaking change in 1.16
[ https://issues.apache.org/jira/browse/FLINK-29872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17628511#comment-17628511 ] Chesnay Schepler commented on FLINK-29872: -- That's not a compatibility direction that we support in the first place though. We only make sure that a client can talk to future versions of Flink, not previous versions. In particular the REST client has problems with compatibility anyway since it can submit job graphs that are subject to java serialization (breaking any compatibility guarantees anyway). > JobDetailsInfo Rest endpoint breaking change in 1.16 > > > Key: FLINK-29872 > URL: https://issues.apache.org/jira/browse/FLINK-29872 > Project: Flink > Issue Type: Bug > Components: Runtime / REST >Affects Versions: 1.16.0 >Reporter: Gyula Fora >Priority: Major > > Flink 1.16 introduces a breaking change to the JobDetailsInfo endpoints by > adding 3 required fields to: > [https://github.com/apache/flink/blob/master/flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/job/metrics/IOMetricsInfo.java] > accumulatedBackpressured, accumulatedIdle, accumulatedBusy fields should not > be primitive (which makes them required) but instead nullable. > This would allow the 1.16 restclient to read the jobdetails of previous > cluster versions -- This message was sent by Atlassian Jira (v8.20.10#820010)
