XComp edited a comment on pull request #18963: URL: https://github.com/apache/flink/pull/18963#issuecomment-1057926590
Thanks for @rkhachatryan and @dawidwys for sharing your concerns. I agree that it's a bit of overkill for this specific usecase. See my inline comments below: > 1. TestingFileSystem might be brittle because it has a lot of unsupported operations. Can't we delegate everything except delete to some "real" FS? The initial intention of `TestingFileSystem` was to have a "framework" to test against in the future. Having all the method throw `UnsupportedOperationException` if not specified forces the developer writing a test to define exactly what behavior to expect from the `FileSystem`. For default behavior, having a temporary filesystem would be good enough. > 2. Callbacks make it less readdable, and mutable callbacks - error-prone, why do we need to reset them? The idea of resetting them to make each test define the exact context it's running in. Where do you see that it's error-prone? That just would indicate that the test's isn't defined precisely. Ideally, we would have a `FileSystem` being specified through a `Builder` pattern and created per test. But the lifecycle of the `FileSystem` should happen outside (i.e. around) the test to make sure that it's get removed again, afterwards. This prevents us from initializing the `FileSystem` within the test. Therefore, I tried this approach with mutable callbacks. > 3. I'm wondering whether a local FS can be picked up instead of a testing FS for some reason - should we guard against it using some "test://" scheme? I don't understand this proposal. Do you see a way to simulate an error using the local FS? I couldn't come up with a solution here. -- 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]
