[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1378756798 Hi @XComp, I'm sorry I've been gone so long. I had a vacation to pass university exams and then got ill 😬. I answered all your comments and made a fix in recent commit (force-pushed only because of rebase). Have a look please, when you will have time. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1401704044 @XComp @PatrickRen At this time I fixed all comments that was by this MR. If there will be no more remarks, existing commits can be squashed into one and merged to master. After that I will create the same MR, but if Flink 1.16. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1418554554 @XComp Big thanks for the review, your comments were very helpful! @PatrickRen Thanks for merging a MR! And sorry for not responding, I was on a vacation without laptop last week, so, unfortunately, couldn't merge it by myself. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1313157507 > Waiting forever in production code is super sketchy and should virtually never be done. > > The PR is also lacking a sort of problem analysis and explanation for how this fixes the issue. Hi @zentol, I added timeout on wait after interrupt and updated PR description to explain the problem and proposed solution more precisely. Can you check the latest commit, please? -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1328553285 Hi @XComp, thanks for the review! I answered on your comments and added a commit with fix. Have a look when you will be available, please. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1333174160 > But just on a side note (independent of this PR's change): I might be wrong but it feels like we could have implemented `reloadCache` in an async fashion returning a `CompletableFuture`. Because when investigating the call hierachy: We're calling `reloadCache` in a `Runnable#run` which we execute in `ReloadTriggerContext#triggerReload` anyway asynchronously. I'm just mentioning it because I'm curious what your view is on that. Actually it's not pretty clear for me why `ReloadTriggerContext#triggerReload` executes asynchronously (returns a future). Probably to give users (developers of custom reload triggers) more flexibility. But anyway, this API was proposed by @PatrickRen and was accepted in [FLIP-221](https://cwiki.apache.org/confluence/display/FLINK/FLIP-221:+Abstraction+for+lookup+source+cache+and+metric), so I followed the agreement. > we could have implemented `reloadCache` in an async fashion returning a `CompletableFuture` Yea, we could, but the benefits are no so clear for me. As I see it, we would have the same `CompletableFuture.runAsync()`, but in `CacheLoader`. IMHO, this class will just become little more overloaded after that. But actually I can do such change if needed. -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1340494991 Hi @XComp @zentol, thanks for your careful review! I took into account all your comments and created a commit with following changes: - Now `CacheLoader` returns future directly in method `CacheLoader#reloadAsync()`. As a consequence `CacheLoader` has direct control over the thread which executes cache reload operation. - I no more rely on common ForkJoinPool. `CacheLoader` creates `Executors.newSingleThreadExecutor()` in `#open` method that is responsible for the whole reload operation. - Now `CacheLoader` can directly shutdown its executor in `#close`, so I decided to remove close mechanism based on boolean variable checking and keep only the one based on thread interruption. Therefore logic became more clear and straightforward. So now there is only one test on `CacheLoader#close` - Now `InputFormatCacheLoader` executes one load task in main reload thread (in `Cacheloader#reloadExecutor`), so we are saving on creating additional thread for reading from input split. - Improved logging on `CacheLoader#close` a little bit Please take a look on the latest commit when you will be available, looking forward 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [flink] SmirAlex commented on pull request #20919: [FLINK-29405] Fix unstable test InputFormatCacheLoaderTest
SmirAlex commented on PR #20919: URL: https://github.com/apache/flink/pull/20919#issuecomment-1344609982 > After some offline chat with @zentol about it, I came to the conclusion that it's really not the right location for such a big change. We should focus on stabilizing the test here. I moved my proposal into [FLINK-30354](https://issues.apache.org/jira/browse/FLINK-30354). I will do another review of this PR on Monday. Yea, I think this MR is already a little bit blowed up, so such big changes should be in separate ticket. About your proposal: I like the idea of reusing the same thread pool, and especially, clarifying the logic of `CacheLoader`, so it won't deal with interrupts. But I'm not sure about changing `CacheReloadTrigger`s and `CacheReloadTrigger.Context`, because it touches already public API (although added relatively recently). BTW during the [FLIP-221 ](https://cwiki.apache.org/confluence/display/FLINK/FLIP-221:+Abstraction+for+lookup+source+cache+and+metric) development I discussed the design with @PatrickRen and @wuchong. So I think it would be good if they also take a look at your proposal in [FLINK-30354](https://issues.apache.org/jira/browse/FLINK-30354) -- 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. To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
