[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 Hi @tillrohrmann, this PR is now rebased to the latest master, and reworked to incorporate your last comments. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 @tillrohrmann yes, reusing `DispatcherGateway#requestJobDetails` was an option that did occur to me but was also not sure of the redundant cost. But sure, we can avoid adding yet another RPC for now. Thanks for the branch link. I'll rebase onto that. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 Maybe you could rebase your work on top of https://github.com/tillrohrmann/flink/tree/FLINK-7870. That's the branch where I stacked multiple of the pending handler PRs and which I will merge shortly after the 1.4 release branch has been cut. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 Thanks a lot @tzulitai. Will take another look at it and merge it if I have no other comments. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 @tillrohrmann the PR is now rebased. Previous comments have also been addressed. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 Yes, I'll rebase this now. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 I think @zentol is right and we can register the `CurrentJobsOverviewHandler` under `jobs/overview`. Thus, we should add this handler as well. Could you rebase this handler onto the latest master such that we can merge it? ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 @tzulitai, I actually want to re-check whether it's indeed not possible to register the `CurrentJobsOverviewHandler` under `jobs/overview`. If this should indeed be the case, then it will most likely subsume this handler. If not, then we'll register this handler under `/jobs` ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 Hi @tillrohrmann, just to double check, so does the conclusion mean that #4805 subsumes this PR, and this one can be closed? ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 @tzulitai, we actually figured out that we cannot register the `CurrentJobsOverviewHandler` under `/jobs/overview` because it conflicts with `/jobs/:jobid`. Therefore, we thought that we could register the `CurrentJobsOverviewHandler` under `/jobs`. This would then not only return the `JobIDs` but also more information about the job. In the future we might add an include query parameter with which one can select which information to return. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 @tillrohrmann @zentol alright, thanks for the review and inputs. I'll address comments and change the `CurrentJobIdsHandler` to return all available Job IDs as a simple list as Till suggested. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 I just talked to @zentol and we both agreed that we should register the `CurrentJobsOverviewHandler` under `jobs/overview`. Moreover, we should change the `CurrentJobIdsHandler` such that it simply returns all available job ids as a simple list. Maybe we could rename it into `JobIdsHandler` or `JobsHandler`. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tillrohrmann commented on the issue: https://github.com/apache/flink/pull/4734 I agree with @zentol. Moreover, I would like to change `MultipleJobsDetails` to not split the job details into running and finished. Just a collection of `JobDetails`. ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4734 Actually, I'd like to throw out the entire handler and replace it with the `CurrentJobsOverviewHandler`. The CurrentJobIdshandler handler is _not_ used by the web UI as the listings of jobs in the UI go through `/joboverview` (that i would prefer to go through `/jobs`). ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/4734 nvm, this isn't used by the web UI: ---
[GitHub] flink issue #4734: [FLINK-7652] [flip6] Port CurrentJobIdsHandler to new RES...
Github user tzulitai commented on the issue: https://github.com/apache/flink/pull/4734 The additional job statuses are only new JSON fields. Not sure how the web UI handles them, but shouldn't the change be non-breaking? We could also revert b60466c for now if that will make things easier. ---
