[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721: URL: https://github.com/apache/hbase/pull/3721#issuecomment-937331616 -- 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
[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721: URL: https://github.com/apache/hbase/pull/3721#issuecomment-937825943 Yes, static method inheriting is always a pain in the java world. You find the problem and provide the PR so I think your opinion is most important. Just want to show the static method approach, do not mean we must go that way, never mind. Let me give another round of review for this PR. > How about updateWithTrackerConfigs? I think updateWithTrackerConfigs is fine, better than the previous two candidates. Thanks. -- 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
[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721: URL: https://github.com/apache/hbase/pull/3721#issuecomment-937804090 This is an approach to force a static method in all the StoreFileTracker implementation classes for persisting the configurations. https://github.com/Apache9/hbase/commit/da63c02057c49a70baafbe6ae533d506f7469a08 The TestStoreFileTracker.testHasPersistConfiguration method is used to make sure that all the implementation classes have the correct static method. @wchevreuil PTAL. I think this could make the normal code path cleaner. For the naming change, updateDescriptor seems too general, what is the reason for changing the method name? I do not mean we can not change it, just want to know the reason. Thanks. -- 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
[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721: URL: https://github.com/apache/hbase/pull/3721#issuecomment-937331616 I just rebased HBASE-26067 against the newest master branch, and seems GitHub can not identify this type of rebase so it will show all the commits in the PR. You can do a cherry-pick to the newest HBASE-26067 and then do a force push to your branch. Sorry for the inconvenience. In the future I will shout before doing the rebase to let others know. -- 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
[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721: URL: https://github.com/apache/hbase/pull/3721#issuecomment-934466509 The UT I mean is to check whether we have the static method for each implementation class. This is possible. I could provide an example later. I think this will be cleaner way. I also want to introduce a init method in the past when implementing migration store file tracker, as we will not always call load when migrating, but finally I just added a check in the StoreFileListFile, to avoid adding extra methods. For me I prefer we keep the interface simple, unless there are no other choices. WDYT? Thanks. -- 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
[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721: URL: https://github.com/apache/hbase/pull/3721#issuecomment-934436283 > We don't have store info at CreateTable procedure time, we don't even have the regions yet by then, so how can we create a RegionFileSystem? Yes, this is a problem. In the first place I thought maybe we could just create a default region and then create a region file system, but it seems to tricky and a bit ugly. So we should let the implementation class to know this. A possible way is to pass in a null StoreContext, and the implementation class should check whether the StoreContext is null, if so, then it should construct itself in the 'persist store tracker configurations' mode. Or another possible way is to force every store file tracker implementation has a static method, for persisting the store file tracker configurations. We could have separated logic in this method, so normal code path does not need to deal with null StoreContext. I think the latter one will be cleaner but not sure if we can force the implementation class has a static method at compile time. But at least, we could introduce a UT for checking this. WDYT? Thanks. -- 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
[GitHub] [hbase] Apache9 commented on pull request #3721: HBASE-26326 CreateTableProcedure fails when FileBasedStoreFileTracker…
Apache9 commented on pull request #3721:
URL: https://github.com/apache/hbase/pull/3721#issuecomment-933571085
In general I do not like that we introduce a new init method but do not call
it when verifying...
I think we should try to provide more things when constructing the
StoreFileTracker. The current implementations are already lazy enough as it
does not actually touch the storage, it just creates some in memory objects...
So why not just pass in a RegionFileSystem for this case? Just like what we
have done in the split/merge procedures.
```
/**
* Used at master side when splitting/merging regions, as we do not have a
Store, thus no
* StoreContext at master side.
*/
public static StoreFileTracker create(Configuration conf, TableDescriptor
td,
ColumnFamilyDescriptor cfd, HRegionFileSystem regionFs) {
StoreContext ctx =
StoreContext.getBuilder().withColumnFamilyDescriptor(cfd).withRegionFileSystem(regionFs)
.withFamilyStoreDirectoryPath(regionFs.getStoreDir(cfd.getNameAsString())).build();
return StoreFileTrackerFactory.create(mergeConfigurations(conf, td,
cfd), true, ctx);
}
```
Thanks.
--
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
