Apache9 commented on a change in pull request #3721:
URL: https://github.com/apache/hbase/pull/3721#discussion_r726098183
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java
##########
@@ -83,10 +84,13 @@ public final void replace(Collection<StoreFileInfo>
compactedFiles,
}
@Override
- public void persistConfiguration(TableDescriptorBuilder builder) {
- if (StringUtils.isEmpty(builder.getValue(TRACKER_IMPL))) {
+ public TableDescriptor updateWithTrackerConfigs(TableDescriptor descriptor) {
Review comment:
I think here we could pass in the TableDescriptorBuilder?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -158,12 +158,12 @@ static StoreFileTrackerBase
createForMigration(Configuration conf, String config
return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx);
}
- public static void persistTrackerConfig(Configuration conf,
TableDescriptorBuilder builder) {
- TableDescriptor tableDescriptor = builder.build();
- ColumnFamilyDescriptor cfDesc = tableDescriptor.getColumnFamilies()[0];
- StoreContext context =
StoreContext.getBuilder().withColumnFamilyDescriptor(cfDesc).build();
- StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true,
context);
- tracker.persistConfiguration(builder);
+ public static TableDescriptor updateDescriptor(Configuration conf,
TableDescriptor descriptor) {
+ //CreateTableProcedure needs to instantiate the configured SFT impl, in
order to update table
+ //descriptors with the SFT impl specific configs. By the time this
happens, the table has no
+ //regions nor stores yet, so it can't create a proper StoreContext.
+ StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true,
null);
Review comment:
I think here we could just check whether the TableDescriptor already has
TRACKER_IMPL set, if so just return to original TableDescriptor, otherwise, we
initialize the store file tracker, and call its updateWithTrackerConfigs method.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -158,12 +158,12 @@ static StoreFileTrackerBase
createForMigration(Configuration conf, String config
return ReflectionUtils.newInstance(tracker, conf, isPrimaryReplica, ctx);
}
- public static void persistTrackerConfig(Configuration conf,
TableDescriptorBuilder builder) {
- TableDescriptor tableDescriptor = builder.build();
- ColumnFamilyDescriptor cfDesc = tableDescriptor.getColumnFamilies()[0];
- StoreContext context =
StoreContext.getBuilder().withColumnFamilyDescriptor(cfDesc).build();
- StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true,
context);
- tracker.persistConfiguration(builder);
+ public static TableDescriptor updateDescriptor(Configuration conf,
TableDescriptor descriptor) {
Review comment:
Let's also name this method updateWIthTrackerConfigs?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/MigrationStoreFileTracker.java
##########
@@ -89,14 +90,16 @@ void set(List<StoreFileInfo> files) {
}
@Override
- public void persistConfiguration(TableDescriptorBuilder builder) {
- super.persistConfiguration(builder);
- if (StringUtils.isEmpty(builder.getValue(SRC_IMPL))) {
+ public TableDescriptor updateWithTrackerConfigs(TableDescriptor descriptor) {
Review comment:
Oh, I just realized that, we do not need to implement this method for
MigrationStoreFileTracker...
Please see HBASE-26264, we do not allow a new table to use
MigrationStoreFileTracker, so here we could just throw a
UnsupportOperationException.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerBase.java
##########
@@ -83,10 +84,13 @@ public final void replace(Collection<StoreFileInfo>
compactedFiles,
}
@Override
- public void persistConfiguration(TableDescriptorBuilder builder) {
- if (StringUtils.isEmpty(builder.getValue(TRACKER_IMPL))) {
+ public TableDescriptor updateWithTrackerConfigs(TableDescriptor descriptor) {
+ if (StringUtils.isEmpty(descriptor.getValue(TRACKER_IMPL))) {
Review comment:
And we could do this check in StoreFileTrackerFactory, before caclling
this method.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CreateTableProcedure.java
##########
@@ -291,8 +291,9 @@ private void preCreate(final MasterProcedureEnv env)
}
TableDescriptorBuilder builder =
TableDescriptorBuilder.newBuilder(tableDescriptor);
- StoreFileTrackerFactory.persistTrackerConfig(env.getMasterConfiguration(),
builder);
- tableDescriptor = builder.build();
+ tableDescriptor =
StoreFileTrackerFactory.updateDescriptor(env.getMasterConfiguration(),
+ builder.build());
Review comment:
Why not just pass in the tableDescriptor? It is immutable...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
