[GitHub] [flink] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-630659431 @pnowojski thanks for your remind. I think I have addressed all comments to update the commits. Since current CI is still pending, my local pipeline based on the same commit [d023c50](https://github.com/apache/flink/commit/d023c507a423b31f38203581c8eed27d28cb3a3c) get a [green result](https://myasuka.visualstudio.com/flink/_build/results?buildId=60=results). 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] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-629180857 I think current IT cases do not target to verify the cancel notification, I'll create one to target to veriy 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] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-629017420 For the 2nd question @romanovacca , after discussed with @zhijiangW offline, we believe this PR with cancellation via RPC would not take part in the data transmission in channel, thus the upstream task would be unblocked once the channel is unblocked which is the same logic as before. 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] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-628828516 @rkhachatryan thanks for your review. It's quite late for me at my time now and plan to update at my day time. Before go to sleep, I could answer the 1st question: barrier alignment would still happen as usual, current PR would not affect barrier align part as discussed in the beginning, and we could improve this 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
[GitHub] [flink] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-628754467 @flinkbot run azure 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] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-626524792 @rkhachatryan Would you please review this PR again? I have split into two commits now and rebase with latest code. 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] Myasuka commented on pull request #8693: [FLINK-8871] Support to cancel checkpoing via notification
Myasuka commented on pull request #8693: URL: https://github.com/apache/flink/pull/8693#issuecomment-621674425 @rkhachatryan Thanks for your review! I'll rebase master code on top of https://github.com/apache/flink/pull/11721 recently. 1. > does it make sense to split the commit (or even PR) into several pieces addressing JM and TM separately? I also previous single commit is a bit large, I'll split into several smaller ones. 2. > can you add a component name to the commit message? Sure, no problem. It was my fault to missing component information. 3. > AsyncCheckpointRunnable.cancelled seems not to be used. And why not to add a new AsyncCheckpointState instead (or reuse DISCARDED)? Previous `AsyncCheckpointRunnable.cancelled` is used for testing to verify whether this async runnable is cancelled. The reason why I not add a new `AsyncCheckpointState` is due to I reuse `AsyncCheckpointRunnable#close` to cancel the running thread which would set the state as `DISCARDED`. I think this is an open question to discuss to search for a better solution. 4. > I don't see how AsyncCheckpointRunnable is canceled or its thread is stopped; which is the original goal of this PR I believe As I said above, I reuse `AsyncCheckpointRunnable#close` to cancel the running async checkpoint task. 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
