KiteSoar commented on PR #17661:
URL: https://github.com/apache/hudi/pull/17661#issuecomment-3694024484
> Hi @KiteSoar , I just did another round of review. The approach looks good
in general while I have some concerns about introducing a new util method that
accepts `Object[] params` to instantiate storage classes. We should assume that
a storage implementation only has constructor that takes `{StoragePath.class,
StorageConfiguration.class}`
Hi @CTTY,
Thanks for the review! I agree with your concern about the `Object[] `params
approach. You're right that we should assume storage implementations only have
the standard constructor `{StoragePath.class, StorageConfiguration.class}`.
I've updated the implementation to use`
HoodieStorageUtils.getStorage(StoragePath, StorageConfiguration)` consistently.
However, I found that in some test scenarios, we need to directly use `new
HoodieHadoopStorage(FileSystem)` constructor instead of the factory method.
Example scenario: In tests like `TestFSUtilsWithRetryWrapperEnable.java,` we
construct custom FileSystem wrappers (e.g., `HoodieRetryWrapperFileSystem`
wrapping a FakeRemoteFileSystem) to simulate specific behaviors like retries or
failures. When using the factory method getStorage(path, conf), it internally
calls FileSystem.get(path, conf) which creates a new FileSystem instance
instead of using our carefully constructed test FileSystem. This breaks the
test logic.
--
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]
