Apache9 commented on pull request #2413: URL: https://github.com/apache/hbase/pull/2413#issuecomment-694613103
> 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. Yes, let's do the refactoring. 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. For other place such as adoptAbandonedQueues, we still use the global queueStorage. I think this could make our code cleaner. I think this can be done before this PR, as a separated issue, to make the PR here smaller and easier to review. > 'Snowflake' it? I'd rather not. Looks like we are planning on many ReplicationSource types. meta+root types would make sense then? Let's not talk about root yet? It has not been implemented in HBase yet. Since we have a special 'metaProvider' field in WALFactory, why not also have a special 'metaSource' in ReplicationSourceManager? I do not think force mixing two completely different things is a good idea here. For example, for metaSource we only care about the wal files from metaProvider but if you add it to the sources map, it will also receive the wal files from other wal providers, then you need extra logic to filter them out. For a special logic, I prefer we also give it a special code path. > 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?). Again, let's not talk about root yet. Adding methods to ReplicationSourceManager is not only about the listener, it is about simplity the implementation. > 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. Got your point but I have a different idea here. In general I think the current approach is good, we'd better reuse the current replication framework as much as we can. But for me, I would like to just reuse the 'replication' code, but not the replication source management code. That means, we should reuse most code in ReplicationSource(as you have one in this PR), 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. And in HBASE-24950, I do have a framework to do root replication without using the replication framework, it is much simpler but highly specialized for catalog family only. You can take a look later after I finished a POC but I do not think it suits here. As in general, I think we'd better also reimplement the normal read replica implementation to not store the replication queue, it is not necessary. 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. For queries about this service, please contact Infrastructure at: [email protected]
