huaxiangsun commented on pull request #2451: URL: https://github.com/apache/hbase/pull/2451#issuecomment-702788348
> I hear you. > > But take a look at the calls to storage. Most calls to queueStorage bypass the ReplicationSource instance and the call goes direct to ReplicationSourceManager and its queue storage. What we pass on init here is not used by ReplicationSource instances. > > Let me remove it. Passing it gives the wrong impression. Great! I also confused a bit as I thought most of queueStorage access is from Source. > > I removed passing queueStorage to ReplicationSource#init. It is never used. I suppose we could go the other way and refactor so queueStorage access always goes via a ReplicationSource instance. This would be a big change. Even then it looks like there are usages of queueStorage that bypass ReplicationStorage instance -- which is fine, but it just makes the model of going via ReplicationStorage to do queueStorage incomplete/unclean. Opinions appreciated. > > I took a look at refactoring so we created queueStorage in ReplicationSourceManager Constructor but thereafter, all access to queueStorage had to go via a replicationSource instance. The transition is easy in perhaps 50% of cases. In other contexts, we don't have a replicationSource but just a peerId (could see if could pull ReplicationSource if only a peerId), and then in other places there is no replicationSource instance around. A refactor here is probably warranted but a follow-on AND given that the new CatalogReplicationSource is not party to most of these accesses, the refactor is for sure off-topic. I can file a follow-on... Thanks for the suggestion Huaxiang Thanks for looking into the details! Yeah, we can do refactoring later. ---------------------------------------------------------------- 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]
