DeamonDev commented on PR #27598: URL: https://github.com/apache/flink/pull/27598#issuecomment-3901745258
@rionmonster thats great observation, thanks! I committed changes. As we discussed in another thread, I have currently left the test with the static `TestCommitterMetricGroup` class inside `CommittableCollectorTest`. I think this is a much better design, even though it means making the constructor of the `InternalSinkCommitterMetricGroup` class public. This is mainly because it is then easier to understand the purpose of such test classes by simply looking at the inheritance hierarchy. With the previous solution, there is a risk that someone in future would miss it and write their own version, which would make the codebase less coherent. Secondly, the latter approach produces less boilerplate code. A third solution would be to copy the `InternalSinkCommitterMetricGroup` class and put it under the `$someMavenModule/test` directory as `TestInternalSinkCommitterMetricGroup`. This way, we avoid leaking this class for external usage. OTOH the description of `@VisibleForTesting` annotat ion is quite favorable in order to leave it as it is. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
