wchevreuil commented on a change in pull request #3488: URL: https://github.com/apache/hbase/pull/3488#discussion_r675621273
########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionFileSystemWriteStrategy.java ########## @@ -0,0 +1,220 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.regionserver; + +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.Path; +import org.apache.hadoop.hbase.Cell; +import org.apache.hadoop.hbase.PrivateCellUtil; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.io.Reference; +import org.apache.yetus.audience.InterfaceAudience; + +import java.io.IOException; +import java.util.Optional; + +/** + * HRegionFileSystem write strategy to decouple splits/merge create dir and commit logic + * from <code>HRegionFileSystem</code>, allowing for a plugable behaviour. + */ +@InterfaceAudience.Private +public abstract class HRegionFileSystemWriteStrategy { + + protected HRegionFileSystem fileSystem; + + public HRegionFileSystemWriteStrategy(HRegionFileSystem fileSystem){ + this.fileSystem = fileSystem; + } + + /** + * Returns the directory Path for the region split resulting daughter. + * @param hri for the split resulting daughter region. + * @return a path under tmp dir for the resulting daughter region. + */ + public Path getSplitsDir(final RegionInfo hri) { + return new Path(getParentSplitsDir(), hri.getEncodedName()); + } + + /** + * Defines the parent dir for the split dir. + * @return + */ + public abstract Path getParentSplitsDir(); + + /** + * Defines the parent dir for the merges dir. + * @return + */ + public abstract Path getParentMergesDir(); Review comment: > I think it might be cleaner to push the RegionInfo into this call. Let the implementation decide where to put the "region dir" it's creating. So this is called by `HRegionFileSystem.getMergesDir`. In the original implementation, what `HRegionFileSystem.getMergesDir` returns is a parent dir of the result merging region dir. Implementations are deciding where result merging region dir is gonna be created (`TBL_DIR/R1/.merges` for the default strategy, `TBL_DIR/` for the direct store one). > The "getParent..." terminology is a little bit confusing to me. I think I really just want to know if I'm splitting or merging a region, where does the new region(s) get created? :) Eh, I got confused by the original naming. When looking at `HRegionFileSystem.getMergesDir`, `HRegionFileSystem.getSplitsDir`, I was expecting those to already give me the merge/splits paths, not just the parents where those were to be created. I can rename these methods accordingly, though. -- 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: issues-unsubscr...@hbase.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
