[GitHub] [hbase] saintstack commented on pull request #2413: Add a new ReplicationSource for hbase:meta WAL files. The new

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

saintstack commented on pull request #2413:
URL: https://github.com/apache/hbase/pull/2413#issuecomment-694407805


   Thanks for the review.
   
   `But in general, I think why we introduce a WALFileLengthProvider is that we do not want to reference WALFactory and WALProvider directly in the replication code so in the future it will be easier to decouple them, but in this PR we add both WALFactory and WALProvider references to the replication code, which is not good...`
   
   Sorry. Is this written up anywhere? I didn't come across it in code. Perhaps I missed it.
   
   The WALFileLengthProvider as implemented presumed a single WALProvider only. The WALFactory is passed because it knows of all WALProviders.
   
   WALProvider does file length but also filtering of WALs and WAL Edits.
   
   `Instead of adding a new isQueueStored flag, we could just implement a ReplicationQueueStorage which stores nothing? Then we do not need to add checks everywhere.`
   
   Storage is in the ReplicationSourceManager, not in the ReplicationSource and is apart from ReplicationSource (you add a ReplicationSource and then you do storage machinations). Maybe I could refactor and pass Storage to ReplicationSource and let it figure if it wants to Store or not.
   
   `
   Since the ReplicationSource for meta is a special one, we do not need to add it to the replication source map in ReplicationSourceManager, just give it a special field.`
   
   'Snowflake' it? I'd rather not. Looks like we are planning on many ReplicationSource types. meta+root types would make sense then?
   
   `Passing the WALProvider for meta when calling the addHBaseMetaSource method, instead of changing lots of constructors and initialization methods in the replication related code base to pass WALFactory. Or maybe we just need to add a special WALActionListener to WALProvider for meta, if meta replication is enabled, we call some special method in ReplicationSourceManager(maybe introduce preMetaLogRoll and postMetaLogRoll methods to ReplicationSourceManager?), then we even do not need to add the addHBaseMetaSource method? In ReplicationSourceManager, if preMetaLogRoll is called and we find out that there is still no replication source for it, we just create one. Of course we still need to the removeHBaseMetaSource method, as we will not close WALProvider for meta even if meta region has already been moved out so we can not implement the logic through WALActionListener(maybe it will be good if we implement this logic? Not sure).`
   
   Hmm.
   
   I tied ReplicationSource and WALProvider explicitly (the WALFactory is passed only because it knows what WALProviders are available in the system... I could undo this I think though it gets tricky around ReplicationSourceFactory). WALProvider does WAL length support, and filtering of WAL files and WALEdits. Seems natural that a ReplicationSource take one of these? (The code as it was presumed one provider only which didn't work when trying to add in meta WAL replications and won't work when other providers as seems to be hinted at above).
   
   I could work on undoing the WALFactory passing but ok to pass WALProvider and for ReplicationSource to have one of these?
   
   WALProvider for meta is lazily instantiated as is the ReplicationSource for meta. This makes the implementation a little more involved especially around adding listeners. The add/remove of the meta ReplicationSource being done inline w/ the open/close/move of hbase:meta Region seems clean compared to moving listener adding into the meta WALProvider and adding methods on ReplicationSouceManager just for the meta WAL Provider (When ROOT shows up, we add methods for it too?).
   
   But let me look at your suggestion more...
   
   I was trying to see if I could break out hbase:meta region replica from replication but did not see a clear path as yet. I mostly made it do without peer references (though not completely... as peer is baked in deep into this replication stuff). When I was not able to break out region replicas, I tried to NOT special-case meta ReplicationSource but make it work like any other.
   
   So, a few things:
   
    * Sounds like there are designs on this part of the code that I need to read up on... Pointers appreciated. I'd like to have it so this meta region replica stuff works w/ whatever those plans are and. not get in their way.
    * This stuff is complicated. It would benefit from any attempt at simplification.
   
   Thanks for taking the time to review.
   
   


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