[GitHub] [flink] TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API
TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API URL: https://github.com/apache/flink/pull/10311#issuecomment-559796472 travis fails unstably on a known issue https://issues.apache.org/jira/browse/FLINK-14894 which cannot be reproduced locally merging now... thanks for your review! This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API
TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API URL: https://github.com/apache/flink/pull/10311#issuecomment-559643033 tison 10:48 AM I narrow the change set to only unwrap accumulator inside client codes. Here is the diff 6d8f1afa7026a6bbab7a3d024fc67b8588e8f522 So the remain concern from my side is about core variant of JobStatus . I will be ok if you can describe how we deal with these two JobStatus in the future. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API
TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API URL: https://github.com/apache/flink/pull/10311#issuecomment-559635706 cherry-pick from slack channel. feel free to react wherever you like. >Sorry but when rebasing I cannot convince myself about why we introduce a flink-core variant of `JobStatus`? `ClusterClient` will return runtime `JobStatus` while `JobClient` returns `JobStatus`. It doesn’t make sense to me for introducing such different. >Runtime version `JobStatus` doesn’t depend on anything inside runtime but a self-contained enum. Shall we add it into `o.a.f.api.common`? Different from ClosureCleaner which could be used by connectors I think `JobStatus` is previously totally internal concept that should not breaks user setups and dependencies if we move it. >I’ve pushed a set of commits that we all agree on. The remain problem is about `getJobStatus` and `getAccumulator` > >for `getJobStatus` the main concern is about where `JobStatus` stays and whether we introduce a variant of `JobStatus`. My opinion is above. for `getAccumulator` the main concern is about whether Flink does unpack job for the user. I think we can do so, but maybe in another pass of pull request so that we firstly move forward this set under consensus. So my idea is that we commit this set of commit as part 1 of FLINK-14762 and I start a new pull request refactor `getAccumulator` and then implement its `JobClient` interface. While let’s align about `JobStatus` . Another coin about `JobStatus` is that we already display this sort of status on WebUI so it is reasonable to be core/common api(at least it is effectively user-facing). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API
TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API URL: https://github.com/apache/flink/pull/10311#issuecomment-559553897 Address comment. @aljoscha please take a look about the update as well as my reply above. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API
TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API URL: https://github.com/apache/flink/pull/10311#issuecomment-558414935 > Also we could simply return `CompletableFuture` from the `JobClient.cancel()` method, instead of moving classes around. On this, I do not have a strong opinion, but do you think that there is any particular reason why returning an `Acknowledge` instead of `Void`? I've ever thought of `Void`. `Void` should work well atm. My concern is 1. If we keep in mind the possibility that a rpc based implementation of JobClient, for current implementation akka doesn't allow null message. Although we don't stick to use akka as rpc implementation, a non-null unit value is better than null representing `Void`. 2. Unfortunately Java doesn't have a builtin non-null unit value so that many of Java projects have to implement their own. I think `Acknowledge` its Flink's unit value which is reasonable to move into flink-core. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [flink] TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API
TisonKun commented on issue #10311: [FLINK-14762][client] Enrich JobClient API URL: https://github.com/apache/flink/pull/10311#issuecomment-558413695 > Hi @TisonKun , thanks a lot for the work. As an overarching comment, I noticed that you add a flag `moveOwnership` to the `ClusterClientJobClientAdapter` and this also bubbles up till the `ClusterClient.submitJob()`. > > I am wondering if this is needed, and from a conceptual point of view, I lean towards the _no_. As a first point, I noticed that the only places that this is set to `false` are the `ClientUtils.submitJob...` which are mainly used in tests. And second, why not closing the `ClusterClient` always, when the `JobClient` closes (which is the responsibility of the call-site), and just let the user decide when to close the job client. > > If the user wants to do something with the `JobClient`, he/she can create a new one (although we still need to figure out how to "retrieve" a `jobClient`). > > WDYT? Yes I also tend *not* to do so. At that moment I was a bit delirious for thinking about whether or not close cluster client on job client closed :/ for shutting down things, I think it is still a configurable action whether or not we close cluster client on job client close because cluster client "spawns" job client and maybe we call `submitJob` multiple times within one cluster client(normally for job management platform). Neither we want to spawn cluster client per job nor we want to close the shared cluster client on job client closed. The point here is "who is responsible for closing cluster client? job client or the caller?" I push a commit daf85a75b9c24918058b8bfe09416b2828bd02a5 for customizing actions on closed for define such manner while considering code quality. Does it make sense to you? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
