saintstack commented on pull request #2413: URL: https://github.com/apache/hbase/pull/2413#issuecomment-698113652
I deleted the backing HBASE-20505 branch in my repo to replace it w/ updated version that addresses comments in here and made it against the HBASE-11870 branch at #2451 . This action looks to have closed out this PR. #2451 takes over from this PR going in a direction suggested in here. Going over @Apache9 's remarks again... > 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 #2451 preserves the WALFileLengthProvider. > I wonder whether we really need this flag here, just set queueStoreage to null, or have an Optional? In #2451, the ReplicationSourceInterface gets a new method, logPositionAndCleanOldLogs. The default implementation is the current call through to ReplicationSourceManager. In the new hbase:meta ReplicationSource, it just makes this call a noop. This is only time the new ReplicationSource might use queue storage; it doesn't register as a general ReplicationSource and doesn't go via general create/add/init/start so it skirts all other areas where there is a write to queue storage. > We have a queueStorage in ReplicationSource, so in ReplicationSourceManager, if we have a ReplicationSource when we want to store or load something from queueStorage, we could use the one in the ReplicationSource. Couldn't do this. Most of the calls to queue storage by-pass ReplicationSource altogether going straight to ReplicationSourceManager (see above where we change this in one case only to insert ReplicationSource for the one call that matters to the new hbase:meta ReplicationSource). > After rethinking, I think there are several ways to simplify the implementation. The above is followed by three suggestion. The first does not apply because #2451 does a version of the second item. The third item doesn't apply either having taken suggestion number two. > Could make this ReplicationPeer a static one? Couldn't make it static. Made it a special class. > ...but better to have a special code path in ReplicationSourceManager to add/remove the replication source, to receive new wal files etc, as I do not think it will add too much code, and could leave the original code as is to not confuse developpers... #2451 does this > but once we move meta region out, the WAL will not be closed. I think with your approach in this PR, to add something when assigning/unassigning meta region, we could also clean up the meta WAL? And it could be a separated issue before this PR, to also make this PR smaller. #2451 does not do this (Don't WALs auto-close after a period w/ the period shortened for hbase:meta? I seem to remember something like this...) > And after rethinking, if we want to support splittable meta(no matter which way we choose), then add/remove meta source when assign/unassign meta region will be much complicated, as we need to do reference counting and there could be race as the design of assign/unassign is that different regions can run concurrently. #2451 does lazy instantiation of the hbase:meta ReplicationSource just after creation of MetaWALProvider if the configuration dictates Region Replicas for hbase:meta. Once up, the ReplicationSource sticks around till the process dies so as to cater for hbase:meta Region being moved back to this server or to cater to N hbase:meta Regions. > And for ReplicationSourceFactory, I suggest that we do not touch it. #2451 does this ---------------------------------------------------------------- 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: [email protected]
