dmvk commented on pull request #15904: URL: https://github.com/apache/flink/pull/15904#issuecomment-842354471
Hi Chesnay, thanks for the review! Nice catch with the `suspend`, I'll move the metrics de-registration there 👍 I'll try to make the test more robust / understandable tonight and cover `FineGrainedSlotManager` as well. It would be great if we could make this into 1.13.1 release, as this is a regression from 1.12.x branch. > I would suggest to change the test such that it has a separate thread querying the metric. You'd need to intercept the initial registration, and cancel the loop once the metric is being unregistered. It's little bit unclear to me, how separate thread helps to test this. The only code path, that it was supposed to stress is accessing metrics in `MetricRegistry#unregister` (this is the actual issue, we run into in Beam runner). To make test more robust: - I'll assert that register / un-register for particular metric has actually been called. (eg. for `MetricNames#TASK_SLOTS_AVAILABLE`). This should also solve the possible problem with new metrics in the future. - I can assert metric value during un-register, to make sure we still have access to a correct value. > it exhibits behavior from the metric registry that in this form doesn't exist; it never throws exceptions nor will it ever call back into the metric when it is being unregistered. I'm not sure what exactly you mean by this comment. This behavior is exactly what [Beam FileReporter](https://github.com/apache/beam/blob/master/runners/flink/src/main/java/org/apache/beam/runners/flink/metrics/FileReporter.java#L72) does in portable runner. If you think, there is a misuse of the API on Beam's side, please let me know. WDYT? -- 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
