[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-737624706 > Although the `ProgramInvocationException` is not annotated as `Public` or `PublicEvolving`, due to the fact that it has been here since forever, it seems that in some places we may be handling it specially and even expose it to the users. For example, the `ClientUtils.executeProgram()` is public and in the mailing list in the past there were people saying that they use it in other projects (I think Beam used to use it and I do not know if they still do). > > On a separate note, given that this PR although small it has taken more than 3 weeks and a lot of discussions, I am starting to doubt if we should move forward with it. For example, the question that @tillrohrmann mentioned to have a separate exception for the runtime components and a separate for the client so that we can handle them differently, seems a deeper issue that requires more thought. > > I may be wrong on that, but the discussions seem to be disproportionately long for the size and also the added value of this PR. What do you think? > > BTW sorry for your time @SteNicholas if this does not get merged in the end but when I opened the JIRA I did not have a clear view of all the related changes that would pop up. @kl0u , IMO, I really want to follow the remaining comments from @tillrohrmann , and continue to update the commit according to Till's above comments. It's better for me to merge this pull request in the end. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-737624582 > example @kl0u , IMO, I really want to follow the remaining comments from @tillrohrmann , and continue to update the commit according to Till's above comments. It's better for me to merge this pull request in the end. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-736948175 Thanks for @tillrohrmann detailed review reply. I agree with the point that `JobExecutionException` transporting the final `ApplicationStatus` shouldn't be used within the runtime components. I would change the commit in `ExecutionGraphBuilder` to remove the `JobExecutionException` from `runtime ` components. BTW, @aljoscha , could you please give your opinion about the changes to `CompletionExcetpion` contract for `JobClient`? 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-733035458 > Thanks for creating this PR @SteNicholas. In general I like the unification of thrown exception. I had two questions: > > 1. Is the `ProgramInvocationException` part of the public API of a `JobClient`? > 2. Can we remove the `JobExecutionException` from the runtime components (specifically the `ExecutionGraphBuilder`)? @tillrohrmann , thanks for your detailed review. My answer for above question you mentioned is as follows: 1.ProgramInvocationException isn't the part of the public API of a JobClient. Even if it is, it is wrapped into CompletableFuture. 2.Why the JobExecutionException is removed from the runtime components (specifically the ExecutionGraphBuilder)? What are the adverse effects of this？ 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-733034701 > 2\. ExecutionGraphBuilder @tillrohrmann 1.`ProgramInvocationException` isn't the part of the public API of a `JobClient`. Even if it is, it is wrapped into `CompletableFuture`. 2.Why the `JobExecutionException` is removed from the runtime components (specifically the `ExecutionGraphBuilder`)? What are the adverse effects of this？ 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-731067152 @tillrohrmann , could you please review this pull request for the application status of `JobExecutionException? 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-731046963 @tillrohrmann , could you please review this pull request and take a look at the [commit](https://github.com/kl0u/flink/tree/client-exception-fin) of @kl0u ? 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-728748152 > @SteNicholas I had a look at this solution and I would like to think on how to make the `status` `final`. This means that I am trying to see if we can have all the `JobExecutionExceptions` with a `status`. I will get back with more comments tomorrow. As you mentioned, all the `JobExecutionExceptions` could be with a given `status`. I have tried to make `status` final and updated `JobCancellationException`, `JobInitializationException` and `JobSubmissionException`. IMO, the `status` of `JobInitializationException` and `JobSubmissionException` is UNKNOWN, and the `status` of `JobCancellationException` is CANCELLED. Please check this. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-727921163 > @SteNicholas why not putting the `status` in the `JobExecutionException` and remove the `JobExecutionResultException`? Because of the compatibility for the previous `JobExecutionException`. I have pushed commit that add the `status` to `JobExecutionException` to check wheter the commit could work. Please review this again. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-727142827 > I am having a look right now @SteNicholas @kl0u Any other comments by 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-726555110 > Thanks a lot @SteNicholas. Feel free to take the commit and integrate it in your PR :) @kl0u Thanks for you better implementation commit. I have integrated into my pull request. Please help to review again. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-725983640 > Hi @SteNicholas , I have a commit with my comments here https://github.com/apache/flink/tree/client-exception. Also for the tests I think we should just add tests for all the cases in the `JobResultTest`. What do you think? @kl0u Your implementation is better for me than my bad implementation. And the test cases look clear for each client. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-725907605 > Hi @SteNicholas , I reviewing your PR now. One thing to add is some tests (for each client) that verify that all clients throw the expected type of exception. This is to make sure that no one breaks the contract in the future. Also this can become part of the javadoc of the `JobClient.getJobExecutionResult()`. I forgot to add `JobExecutionResultException` contract to the javadoc of `JobClient.getJobExecutionResult()`. And about the tests, I thought that current test cases include the tests that verify that all clients throw the expected type of exception. But it's better to add tests for each client extrally. 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
[GitHub] [flink] SteNicholas commented on pull request #14028: [FLINK-20020][client] Make UnsuccessfulExecutionException part of the JobClient.getJobExecutionResult() contract
SteNicholas commented on pull request #14028: URL: https://github.com/apache/flink/pull/14028#issuecomment-725301089 @kl0u , could you please help to review this commit if you are available? 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
