HBASE-24474 Rename LocalRegion to MasterRegion #1811
Conversation
|
@ndimiduk PTAL. Thanks. |
|
The new useMetaCellComparator parameter is for storing root table in the future. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
| params.flushIntervalMs()); | ||
| conf.setInt(AbstractFSWAL.MAX_LOGS, params.maxWals()); | ||
| if (params.useHsync() != null) { | ||
| conf.setBoolean(HRegion.WAL_HSYNC_CONF_KEY, params.useHsync()); | ||
| } | ||
| if (params.useMetaCellComparator() != null) { |
There was a problem hiding this comment.
We need to add this in here?
There was a problem hiding this comment.
For used as procedure store, this is not necessary. This is for preparing for storing root table in the future, as for root table, the row key is the same with meta table, and we need to use a special comparator. I think it is fine to change the comparator in the future?
| } | ||
|
|
||
| public static LocalStore create(Server server) throws IOException { | ||
| LocalRegionParams params = new LocalRegionParams().server(server) |
There was a problem hiding this comment.
How comes we can drop these methods?
There was a problem hiding this comment.
We will return MasterRegion directly after this change, so we do not need these methods any more, users can use the methods in MasterRegion directly.
|
Any other concerns? Need to rebase the patch for HBASE-24389 once this done. Thanks. |
| @@ -450,7 +451,7 @@ public void run() { | |||
| private ProcedureStore procedureStore; | |||
|
|
|||
| // the master local storage to store procedure data, etc. | |||
| private LocalStore localStore; | |||
| private MasterRegion masterRegion; | |||
| @@ -1563,7 +1564,7 @@ protected void stopServiceThreads() { | |||
| private void createProcedureExecutor() throws IOException { | |||
| MasterProcedureEnv procEnv = new MasterProcedureEnv(this); | |||
| procedureStore = | |||
| new RegionProcedureStore(this, localStore, new MasterProcedureEnv.FsUtilsLeaseRecovery(this)); | |||
| new RegionProcedureStore(this, masterRegion, new MasterProcedureEnv.FsUtilsLeaseRecovery(this)); | |||
There was a problem hiding this comment.
Having everything use a single region has me a little nervous. Seems like it'll make it easy for two unrelated subsystems to step on each others' toes later on -- conflicting row keys, columns, &c. This should be fine for initial work though.
There was a problem hiding this comment.
The idea is to use different families. There is a known risk that, if someone stores a lot data in one of the families, it will slow down the start up of the whole HMaster, even if it is not necessary. We should document this in our ref guide. Can be a follow on issue?
| */ | ||
| @InterfaceAudience.Private | ||
| public final class LocalRegion { | ||
| public final class MasterRegion { |
There was a problem hiding this comment.
We still need the wrapper class with delegation? How about have the factory manage creation of the HRegion (wiring up the wal, &c) and HMaster hold the instance of HRegion directly?
There was a problem hiding this comment.
This is intentional. You can see the implementation of MasterRegion.update, we have to call flusherAndCompactor.onUpdate(); after each update. So if we expose the HRegion directly, the callers have to do this by their own, and I believe it will be easy to forget and then cause big problem...
| } | ||
|
|
||
| public static LocalStore create(Server server) throws IOException { | ||
| LocalRegionParams params = new LocalRegionParams().server(server) |
| * Whether to use {@link MetaCellComparator} even if we are not meta region. Used when creating | ||
| * master local region. | ||
| */ | ||
| public static final String USE_META_CELL_COMPARATOR = "hbase.region.use.meta.cell.comparator"; |
There was a problem hiding this comment.
If this configuration point is specific to master side, should it have master in its name?
There was a problem hiding this comment.
For now it is only used in HMaster but I do not think it should be prefixed with hbase.master, as it is a configuration for HRegion.
| * Whether to use {@link MetaCellComparator} even if we are not meta region. Used when creating | ||
| * master local region. | ||
| */ | ||
| public static final String USE_META_CELL_COMPARATOR = "hbase.region.use.meta.cell.comparator"; |
There was a problem hiding this comment.
It's a little nit-pick, but I like my configurations to specify components via .-separator, and use _ for component names. So this might be hbase.master.region.use_meta_cell_comparator.
Of course, this might be conflicting with other configuration names and uses. Would need to look things over to see what else is using the "hbase.master" namespace.
Signed-off-by: Michael Stack <stack@apache.org>
Signed-off-by: Michael Stack <stack@apache.org>
Addendum: HRegion changed size.
Addendum: HRegion changed size.
Signed-off-by: Michael Stack <stack@apache.org>
Signed-off-by: Michael Stack <stack@apache.org>
Addendum: HRegion changed size.
No description provided.