[GitHub] [hbase] anmolnar commented on pull request #2931: HBASE-25395 Introduce PersistedStoreEngine and PersistedStoreFileManager

[GitBox <gi...@apache.org>]

anmolnar commented on pull request #2931:
URL: https://github.com/apache/hbase/pull/2931#issuecomment-796780015


   @taklwu I'm checking your commit which introduced the abstract class `AbstractStoreFileManager`. You introduced 2 things here: an abstract base class and hooks. Inheritence graph looks like AbstractStoreFileManager -> DefaultStoreFileManager -> PersistedStoreFileManager. On the top of that you essentially moved all code from the Default impl to the Abstract.
   
   Revisiting my idea about the abstract class I think we don't need that. Let's revert to your original DefaultStoreFileManager -> PersistedStoreFileManager inheritence with the following modifications:
   - PersistedStoreFileManager needs to override method loadFiles(), call the super at the beginning and do what you have in the hook now. No need to hook here.
   - Similarly in insertNewFilesHook() the extra logic is at the end, so instead of the hook, just override -> call super() -> do the rest.
   - addCompactionResultsHook() - This is where the hook magic comes in handy, because the extra logic happens in the middle. Nice as it is now.
   - loadInitialFiles() - This method has to be completely overriden by PersistedStoreFileManager, no shared logic here.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org