smengcl commented on pull request #1088: URL: https://github.com/apache/hadoop-ozone/pull/1088#issuecomment-647761078
Thanks @elek for the review. See inline: > Thanks to create this patch @smengcl > > There are multiple changes in this patch: I'll revise the jira title to more accurately reflect my changes in a bit. > > 1. start to use SPI / META-INF services --> I am +1 > 2. Change the defaultFs from `o3fs` to `ofs` in some tests --> I can accept it, I guess because we think the `ofs` is a long term solution, but don't know the motivation to do it now. But I can accept it. The defaultFS doesn't seem to affect any existing tests right now since I believe most are using full path (e.g. `o3fs://bucket1.volume1/key1` in `mapreduce.robot`). My idea here is to let docker dev experience use OFS by default. So you can do things like `ozone sh mkdir -p /volume1/bucket2/`. If some tests are using relative path and tried to create files directly under volume or bucket with OFS, some acceptance tests should fail and catch the problem. > 3. Use `ClientProtocol` (which supposed to be an internal implementation detail of `OzoneClientAdapter` instead of using the `ClientAdapterImpl`. > > I think it's a good step to remove the usage of implementation as the main goal with interfaces is to hide the implementation. > > But it's not clear what is the long-term goal with this change. > > A. We can either remove the usage of `ClientAdapter` from `RootedOzoneFileSystem` and use just the pure `OzoneClient` / `ClientProtocol`. In that case we don't need to leak the `ClientProtocol` from the `ClientAdapter`. > > B. Or we can keep the `OzoneClientAdapter` if we need a good interface. Using interface means that we don't cast it and don't retrieve internal implementation, just call the methods without understanding what is under the hood. In this case the `getVolumeDetails` should be added to the `OzoneClientAdapter` instead of makeing it possible to get the `ClientProtocol` > > I am fine with both approaches but I would like to understand the goal. I'm in favor of A. I'll _attempt_ to remove the usage of `OzoneClientAdapter` in OFS altogether then. ---------------------------------------------------------------- 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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
