Apache9 commented on pull request #2413: URL: https://github.com/apache/hbase/pull/2413#issuecomment-695216060
> The configuration will be everywhere on all servers. You are suggesting create MetaReplicationSource on all servers on startup even though a server may not carry an hbase:meta Region? And a WALFileLengthProvider for meta on Replication init... Keep them around to hook to the MetaWALProvider when it is lazily instantiated? (Other questions, do I use a ReplicationSourceFactory creating this MetaReplicationSource...If so, how without mixing in the as-yet not-created WALProvider/WALFactory) That's why I suggest we do this on a feature branch first. We could first get things done and then polish it. I think creating it everytime is the most simple way to get things done first. Agree that if possible we could make it along with the initialization of MetaWALProvider. Do as you like, but if you find that it is a bit tie consuming, I suggest we just use the most simple solution to get the whole thing done first. And for ReplicationSourceFactory, I suggest that we do not touch it. HBaseMetaReplicationSource is a special one, at least in the current PR, it is just a special branch in the factory method so I do not see the necessity to also let the creation go with ReplicationSourceFactory. > So, MetaReplicationSource in ReplicationSourceManager as a special data member rather than as part of the general map of ReplicationSources? I think this will be better. But as it is you that write the code, so you may have different feelings but I suggest that you could try this way. For me I do not like mix things up at the beginning. Usually after I finish the logic separately, I could better know how to unify it with the old logic. > The WALFileLengthProvider is currently supplied by WALProvider. Making it so WALFileLengthProvider instead figures which WALProvider it is backed by is an inversion. It is fine if you really think passing a WALProvider is simpler. At least an interface is easier to be mocked in test and also limit the usage, as it only has one method but for WALProvider it has more methods. And to me, there is a plan to break up replication and WAL, but it will be a long term work. It will be better if you can break the tie as much as possible for now, but it is not a blocker. If it not easy then you can do what you like as it is you that write the code. > I think this shouldn't be too bad if the adding/removing goes via ReplicationSourceManager. It will know when it is ok to shut down the ReplicationSource. I do not think the difficulty is in ReplicationSourceManager. It is in the AssignRegionHandler/UnassignRegionHandler. As they could run concurrently, if we unassign the last meta region on this RS, and then there is an assign of a new meta region run at the same time, maybe the execution order of addnig/removing HBaseMetaReplicationSource could be adding and then removing, and then leads to problem, though ReplicationSourceManager itself is thread safe. > I asked if there a writeup on high-level needs around this area of the code that I may have missed but perhaps there isn't one? This seems to be one objective: As I said above, it is for HBASE-20952, but this is a long term work so should not block the curren work if you find out it is really hard to not pass WALProvider to these classes. 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]
