Ted Yu Not sure why you got a compilation error. Will look..
stack Thanks for the detailed comments. Here are the responses.
Rather than change all new Replication invocations to take a null, why not override the Replication constructor? Your patch would be smaller.
I had considered that but it didn't seem adding a new constructor is justified in the long run. There probably are no consumers of the constructor outside HBase, etc., and adding another constructor means new code to take care of, etc. So although it makes the patch bigger, I think it's okay..
Could there be issues with isFileInUse in multithreaded context? Should currentFilePath be an atomic reference so all threads see the changes when they happen? Do you think this an issue?
There shouldn't be any multithreading issues here. Each ReplicationExecutor thread has its own copy of everything (including currentFilePath), and the getters/setters are in the same thread context.
Do we have to pass in an HRegionServer instance into ReplicationSourceManager? Can it be one of the Interfaces Server or RegionServerServices? Or looking at why you need it, you want it because you want to get at HLog instance. Can we not pass this? Or better, an Interface that has isFileInUse on it?
Yes, I tried to pass the HLog instance to Replication's constructor call within HRegionServer. But the code is kind of tangled up. HRegionServer instantiates a Replication object (in setupWALAndReplication). HLog is instantiated in instantiateHLog, and the constructor of HLog invokes rollWriter. If the Replication object was not registered prior to rollWriter call, things don't work (which means the Replication object needs to be constructed first but the HLog instance is not available yet). I tried fixing it but then I ran into other issues...
But yeah, I like the interface idea. Will try to refactor the code in that respect.