[GitHub] [flink] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-716553566 Thanks @gm7y8 . Additionally, I verified manually that the changes result in the expected behavior. Let's wait for @vthinkxie to get back to us to review the code itself. 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] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-714288781 Thanks, @gm7y8 . It looks good from my side. @vthinkxie: Could you have a brief look over the frontend code diff? I guess there have been some changes after your last 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
[GitHub] [flink] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-713382349 > @XComp I was able to identify the code fix. I working to unit test it had some issue with Flink set up to start a job with checkpoint and savepoint. it might take a day or so as I am doing it for the first time. Hi @gm7y8 , thanks for getting back to us. Let's see if we can finish this before the codefreeze next week. Here a few remarks: * The build is failing for me locally (`mvn -pl flink-runtime-web -Dfast -DskipTests install`) with the following error message: ``` [ERROR] ERROR in src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Element implicitly has an 'any' type because type 'CheckPointDetailInterface' has no index signature. [ERROR] src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Property 'checkpoint_type' does not exist on type 'JobCheckpointsDetailComponent'. [ERROR] src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Property 'checkPointConfig' does not exist on type 'JobCheckpointsDetailComponent'. [ERROR] src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Element implicitly has an 'any' type because type 'CheckPointDetailInterface' has no index signature. [ERROR] src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Property 'checkpoint_type' does not exist on type 'JobCheckpointsDetailComponent'. [ERROR] src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Element implicitly has an 'any' type because type 'CheckPointDetailInterface' has no index signature. [ERROR] src/app/pages/job/checkpoints/detail/job-checkpoints-detail.component.html(25,38): : Property 'checkpoint_type' does not exist on type 'JobCheckpointsDetailComponent'. ``` It's always good run a final `mvn install` at least on the modules you touched to verify your changes. Alternatively (or rather as an additional tool), you can setup Azure CI on your fork. That will run a full test covering all modules for each commit on your fork. Check out the [tutorial for setting up Azure CI](https://cwiki.apache.org/confluence/display/FLINK/Azure+Pipelines#AzurePipelines-Tutorial:SettingupAzurePipelinesforaforkoftheFlinkrepository). * The changes like [job-checkpoints-detail.component.html](https://github.com/apache/flink/pull/13458/commits/0b8e369aa5c8c4e4a8c370b4b848c0f197fcdb9d#diff-a1c50fd814fe8a1c03567f9c71877873251223cfaf32c7067068f7f30d8c5c42R25) does not reflect @AHeise's request as you use uppercased tokens here. @AHeise suggested normal casing (i.e. lowercase) as it helps to improve the readability. * Additionally, I realized that you didn't address all points of the PR description template. May you fix that? Thanks. 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] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-711999197 @gm7y8 What's the status? Would it be possible for you to finish this feature within this week? Otherwise, I would take over by Wednesday to finish it prior to the feature freeze for `1.12` probably happening next week. 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] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-708496012 Hi @gm7y8 , please go ahead with implementing the request @AHeise brought up considering the text change @AHeise proposed: > Possible values should be “aligned checkpoint”, “unaligned checkpoint”, “savepoint”, "sync_savepoint" (which I'd rename to "savepoint on cancel" to make it less technical and more user-friendly in the UI). 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] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-703661982 > Thanks for this improvement @gm7y8 > Overall, it looks good to me. > > @XComp @vthinkxie Did someone check that the change works in UI? > > I left comments about changes which are probably unrelated. > After addressing these comments/question, I can merge the PR. > > Also, not sure what the first 'merge' commit means, I can remove it before merging. I ran the Web UI including the changes. It looks like expected. 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] XComp commented on pull request #13458: FLINK-18851 [runtime] Add checkpoint type to checkpoint history entries in Web UI
XComp commented on pull request #13458: URL: https://github.com/apache/flink/pull/13458#issuecomment-703586638 > > Thanks @gm7y8 for your changes. There are only two minor formatting things left which need to be addressed. Additionally, please update the commit message to comply to [Flink's commit format](https://flink.apache.org/contributing/contribute-documentation.html#submit-your-contribution). It should look like `[FLINK-18851][runtime-web] ...`. > > I'm gonna ask @vthinkxie to review the `web-dashboard` changes in the meantime. For me, they look good. > > The frontend code looks good to me Reconsidering the change: @vthinkxie what about exposing this information to the Checkpoint history page as well? 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
