elek commented on pull request #1363: URL: https://github.com/apache/hadoop-ozone/pull/1363#issuecomment-684914342
Thanks for the separation @smengcl, I think it's easier to discuss. I am not sure it's the same patch what I already commented or not. I had a comment in the previous PR, where the discussion is stopped: >> I'm in favor of A. I'll attempt to remove the usage of OzoneClientAdapter in OFS altogether then. > I am fine with that approach but let me add some comments to the latest patch. > The naming of BasicRootedOzoneFileSystem and BasicRootedOzoneFileSystemImpl is misleading. Usually the Impl postfix is used when the class implemented a well known interface. There is no such interface here. (It's more like the delegation design pattern not an implementation) > As a test: Can you please explain what are the differences between the two classes and the responsibilities? > If not, we don't need two classes. Just remove the Impl and remove the dedicated methods and directly call the proxy from the original methods of BasicRootedOzoneFileSystem. > Wouldn't it be more simple? This patch seems to use `BasicRootedOzoneClientAdapterImpl` which is not an `Impl` (not an interface). Do you need a client adapter here (in the old code we need an interface and an implementation for classpath separation but here this separation is removed.)? If yes, do you need `Impl` in the name (it doesn't implement anything)? ---------------------------------------------------------------- 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 --------------------------------------------------------------------- To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
