Thanks Siyao for adding this useful feature. I reviewed several PRs on the feature branch and feel positive to merge it master. Here are a few suggestions:
1. /tmp mount needs better document: Anyone can create a bucket under global /tmp volume in addition to their personal mounted /tmp when using o3 protocol? The two additional /tmp provision steps seem to be different from using HDFS. We should document it clearly. 2. Some follow up work on trash clean up implementation on OM. That can be added after the merge. 3. Some missing features from both o3fs and ofs, such as ACL. This need further discussion to map HCFS acl to ozone acl. I'm OK with adding it post merge. 4. Some refactor work to dedup the FS implementation between ofs and o3fs. Overall, I'm +1 on merge. Thanks, Xiaoyu On Fri, Jun 12, 2020 at 12:02 AM Elek, Marton <e...@apache.org> wrote: > > > Yes I admit there are quite a few duplicated lines of code. And this is a > > problem that needs to be solved sooner or later. > > In the early stage of OFS development I discussed this with @Xiaoyu. We > > figured this could be resolved "later" -- which means now or soon after > > merge. > > At that time (beginning of the feature branch) I was trying to touch as > few > > existing classes as possible to minimize the conflict when rebasing the > > feature branch. This is also partially the reason for the duplication. > Also > > the plan didn't work out as expected. > > Thanks to explain it. I am fine with doing it after a merge, if later > means near-future/before the next release ;-) > > And I understand the motivation to keep the o3fs untouched, but I would > like to encourage you: feel free to modify old code, too. > > > >> We discussed in PR the BasicRootedOzoneFileSystem#adapterImpl can be > > removed by exposing getVolume() in OzoneClientAdapter. > > https://github.com/apache/hadoop-ozone/pull/1021#discussion_r436749198 > > > *ClientAdapters are created to separate OzoneClient/ObjectStore from the > OzoneFileSystem to make it possible to use the FilteredClassloader. > > As the classloader is removed we can consider to remove ClientAdapters, > too (at least from the new code). I think some of my concerns are rooted > in the approach which tried to keep the previous structure. Again: feel > free to do refactors if it makes it simpler / cleaner code. > > I also learned that the ClientProxy might be required to get better > performance (ObjectStore/getVolume/getBucket executes a new GetBucket > request all the time). > > As of now it is marked as @VisibleForTesting. We might need to remove > the annotations and use it as now. > > > Summary: I am fine with merging it to the master (it couldn't break > anything), but I would prefer to continue the discussion and have an > agreement what should we do before the next release... > > And I propose to run at least 4-5 builds in #1021 and analyze all the > failures (!), before the merge (As I understood > 2eb3181b552824ea8a5e5956387e4c8b540b97c9 from the #1021 is the proposed > head to merge) > > Thanks, again, all the great work > Marton > > --------------------------------------------------------------------- > To unsubscribe, e-mail: ozone-dev-unsubscr...@hadoop.apache.org > For additional commands, e-mail: ozone-dev-h...@hadoop.apache.org > >
