Uh oh!
There was an error while loading. Please reload this page.
HBASE-25393 Support split and merge region with direct insert into CF… - #3488
HBASE-25393 Support split and merge region with direct insert into CF…#3488wchevreuil wants to merge 8 commits into
Conversation
| * <code></code>MergeRegionStrategy</code> implementation to be used in combination with | ||
| * <code>PersistedStoreEngine</code> to avoid renames when merging regions. | ||
| * | ||
| * To use it, define the following properties under master configuration: |
There was a problem hiding this comment.
I do not think this should be a master level config? Since different region could have different StoreEngine implementations(though I do not think we should have a PersistedStoreEngine but that's another story), we can not use the same merge strategy for them at master side?
There was a problem hiding this comment.
Good point. Let me modify this. I guess for consistency, maybe table level configuration, rather then family level.
There was a problem hiding this comment.
Could cause confusion/inconsistencies, no? Same region splitting using different approaches to different stores?
There was a problem hiding this comment.
Changed to load both mergestrategy and write strategy from the table config.
Apache-HBase
commented
Jul 14, 2021
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Jul 15, 2021
And in general, I do not think creating split/merged region in a tmp directory first is related to how to track the store files. Unless we are using HBCK to scan the file systems to recover hbase:meta, otherwise we will just trust what we have in hbase:meta, so creating regions directly in the final place is not a problem here. I guess why we create it in tmp directory first is because the CatalogJanitor? Not sure. But theoretically, since we do not need to scan the filesystem to get all the regions for a table when opening it, I do not think we need to introduce a Strategy to handle both cases, just change the code to write it to the final place. This could be done on master and branch-2. Thanks. |
Change-Id: I8704084516e865bf4a3b3a7adbab6dc635a31c6e
wchevreuil
commented
Jul 15, 2021
I guess hbck recovery could be a problem for the default tracking, if the resulting merging/splitting region dir got already created in the FS, fails before completing the operation then vanishes from meta. CatalogJanitor shouldn't be a problem, as it just cleans out what it can see as "outdated" in meta table, so doesn't really care about these temp dirs. So changing the default behaviour to always write directly would require a review of hbck. I don't have more context on the motivations for the temp dirs usage, so thought about keeping the current behaviour, but allow it to be plugable. |
Apache-HBase
commented
Jul 15, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 15, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 15, 2021
💔 -1 overall
This message was automatically generated. |
…/de-serialization Change-Id: I2d24d71d6d40ea70f0fccf42d3ab83000bd6ef88
Apache-HBase
commented
Jul 21, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2021
💔 -1 overall
This message was automatically generated. |
Change-Id: I401d736eb3004c4e8a1fd43a0adf1be908d1178f
Apache-HBase
commented
Jul 21, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 21, 2021
🎊 +1 overall
This message was automatically generated. |
joshelser
commented
Jul 21, 2021
That's a fun thought. Things like rebuilding meta from the filesystem suddenly become dubious. I think we also know that building and HBCK which is capable of identifying a "split-in-progress" is hard. Like Duo say, we can put the Region in the "correct" place and it will just be used once meta is updated. What if we had some special marker in the Region which we cleaned up after it was opened the first time? HBCK could look for that marker and know that if we find a Region on the FS with this marker that isn't in meta, we should be able to just ignore/delete it (as a split/merge which was in-flight). |
joshelser
left a comment
There was a problem hiding this comment.
I think I need to give this another pass, but thought I'd publish what I have now.
| abstract protected HRegionFileSystem innerMergeRegions(MasterProcedureEnv env, FileSystem fs, | ||
| RegionInfo[] regionsToMerge, Path tableDir, RegionInfo mergedRegion) throws IOException; |
There was a problem hiding this comment.
Should try to come up with a better name for this.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| for (int i = 0; i < regionsToMerge.length; i++) { | ||
| regionsToMerge[i] = ProtobufUtil.toRegionInfo(mergeTableRegionsMsg.getRegionInfo(i)); | ||
| } | ||
| createMergeStrategy(mergeTableRegionsMsg.getMergeStrategy()); |
There was a problem hiding this comment.
Need to guard against the old MergeTableRegionsStateData message not having this newly-added attribute, and default to a specific implementation (either from explicit Configuration or default value)
| * <code>HRegionFileSystemWriteStrategy</code> implementation to be used in combination with | ||
| * <code>PersistedStoreEngine</code> to avoid renames when splitting and merging regions. |
There was a problem hiding this comment.
This makes me wonder if we should just have the WriteStrategy/MergeStrategy implementations defined in the StoreEngine implementation itself. Is there any benefit to having them separate?
I can't think of a reason (besides working around bugs) why we would want no-renames for flushes and compactions but not splits/merges.
There was a problem hiding this comment.
I agree. We have HBASE-25396 in the roadmap, which is about sanitising all these pluggable components that should work together, so was not concerned with that in this specific jira.
| @InterfaceAudience.Private | ||
| public class DirectStoreFSWriteStrategy extends HRegionFileSystemWriteStrategy { | ||
| private StoreFilePathAccessor accessor; | ||
| private Map<String, Map<String,List<Path>>> regionSplitReferences = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Is the ConcurrentHashMap necessary? Multiple concurrent splits accessing this?
Also, who cleans up the entries in this map?
There was a problem hiding this comment.
Is the ConcurrentHashMap necessary? Multiple concurrent splits accessing this?
No concurrent access, so no need for ConcurrentHashMap.
Also, who cleans up the entries in this map?
Yeah, this is a mistake. Entries are not being cleaned. Not a problem for now, as each HRegionFileSystem representing region being split is unique for the context of a single SplitProcedure, but better fix that if this is to be used in a different context.
| Path path = (this.fileSystem.regionInfoForFs.equals(regionToMerge)) ? | ||
| super.mergeStoreFile(mergedRegion, regionToMerge, familyName, f, mergedDir, fs) | ||
| : super.mergeStoreFile(regionToMerge, mergedRegion, familyName, f, mergedDir, fs); |
There was a problem hiding this comment.
Couldn't we require the regions to be positional (regionA, regionB) so we didn't have to do this check and flip-flop the argument order?
Also, is n-way region merges handled at a higher level?
There was a problem hiding this comment.
Couldn't we require the regions to be positional (regionA, regionB) so we didn't have to do this check and flip-flop the argument order?
We could assume that since this is the direct store approach, the HRegionFileSystem instance delegating the job for this strategy is wrapping a resulting merged region and the correct order of params are then passed.
Also, is n-way region merges handled at a higher level?
Ain't sure I follow you. You mean multiple regions merge?
| public abstract Path getParentSplitsDir(); | ||
| /** | ||
| * Defines the parent dir for the merges dir. | ||
| * @return | ||
| */ | ||
| public abstract Path getParentMergesDir(); |
There was a problem hiding this 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.
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?
There was a problem hiding this 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.
Uh oh!
There was an error while loading. Please reload this page.
| public void createSplitsDir(RegionInfo daughterA, RegionInfo daughterB) throws IOException { | ||
| Path splitdir = getParentSplitsDir(); | ||
| if (this.fileSystem.fs.exists(splitdir)) { | ||
| LOG.info("The " + splitdir + " directory exists. Hence deleting it to recreate it"); |
There was a problem hiding this comment.
nit, move over to the slf4j marker message
Change-Id: I34334a756c124ce455ea689cefe0a4e59271e967
Apache-HBase
commented
Jul 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 23, 2021
🎊 +1 overall
This message was automatically generated. |
Change-Id: Id0dd8f1745d91bf731561e688bb1d93cbdb2176e
Apache-HBase
commented
Aug 3, 2021
💔 -1 overall
This message was automatically generated. |
Change-Id: I8c7979370034ac6bca92f198e08eee40d7b0a2c8
Apache-HBase
commented
Aug 4, 2021
💔 -1 overall
This message was automatically generated. |
Change-Id: Ice575ce75f7186e7df581b6c8441b62217a86f0a
Apache-HBase
commented
Aug 6, 2021
💔 -1 overall
This message was automatically generated. |
wchevreuil
commented
Aug 19, 2021
This PR is not relevant anymore now that we have the changes of HBASE-26187 merged into master and branch-2. |
This is the "rename-less splits/merges" solution for HBASE-24749.