HBASE-23281: Track meta region locations in masters

[bharathv]

This patch adds a simple cache that tracks the meta region replica
locations. It keeps an eye on the region movements so that the
cached locations are not stale.

This information is used for servicing client RPCs for connections
that use master based registry (HBASE-18095). The RPC end points
will be added in a separate patch.

[bharathv]

Just to highlight major comments from the previous prototype patch here.

[bharathv]

@wchevreuil / @apurtell FYI since you were looking at it too. (Sorry for the comment spam).

[saintstack]

Some nits.

[bharathv]

Test failure doesn't seem related to the patch. Fixed some more checkstyle nits (hopefully last).

[bharathv]

I'm fairly certain that the test failures are not related to the patch. There is a long discussion between @busbey and me in the Slack channel regarding failures like

"Failed to read test report file /home/jenkins/jenkins-slave/workspace/HBase_Nightly_branch-2.1/output-jdk8-hadoop3/archiver/hbase-server/target/surefire-reports/TEST-org.apache.hadoop.hbase.replication.TestMasterReplication.xml
org.dom4j.DocumentException: Error on line 86 of document : XML document structures must start and end within the same entity. Nested exception: XML document structures must start and end within the same entity."

These look like a manifestation of tests timing out (and eventually getting killed) and resulting in corrupt sure fire reports that Jenkins is not able to interpret. It is being tracked in https://issues.apache.org/jira/browse/HBASE-22470

I re-ran the failed tests and they pass for me locally.

[bharathv]

Publishing the comments, will update the patch shortly.

[ndimiduk]

So any znode under /hbase shows up or disappears and we re-build the cache? Seems like this would cause a lot of unnecessary ZK chatter. I really wish the meta replica list was under a node of its own.

[bharathv]

I really wish the meta replica list was under a node of its own.

I was thinking the same.

Taking a step back, this whole logic is implemented to handle the following scenario. Consider a timeline of events (t n+1 > t n)

t1: standby master starts (populateInitialMetaLocations() runs in c'tor). No meta znodes, so nothing is populated in the cache
t2: Active master starts
t3: meta znodes created.

At this point standby's meta cache is stale and it hasn't registered any watchers, so nothing is notified and will remain a zombie forever.

So to fix this problem, populateInitialMetaLocations() registers a watcher on the base znode and also gets the meta znodes. So if the meta znodes turn out to be empty, we at least have a watcher on the base znode when something gets populated later. So the new timeline would be.

t1: standby master starts. No meta znodes, but registered a watcher on /hbase
t2: active master starts
t3: meta znodes created
t4: standby gets a notification that /hbase has changed and it populates the cache.

Do remember that we do not renew the watcher. So t4 is the last nodeChildrenChanged() event.

Now the only problem is in the following sequence of actions.

t1: active master starts
t2. meta znodes created.
t3: standby master starts, reads the meta znodes but also has a watcher on /hbase.

Now in the future if some children of /hbase changes, we have an extraneous invalidate of cache, but thats ok since that is the last time it is going to happen.

Now one may ask, why not do it the following way.

1. meta_znodes = getMetaZnodes();
2. if (meta znodes == empty) {
   register a watcher on /hbase
3. }

Unfortunately this is not atomic. Between (1) and (2), the meta znodes could be created and standby could remain stale for a while until some children of /hbase changes.

Sorry for the long post. Hope this explains my line of thought. Feel free to point anything that I'm missing here. :-)

[bharathv]

One additional thought, what if some other thread registers a watcher on the base znode and the cache thread processes them (guess this is possible). should we make populateInitialMetaLocations() idempotent? That solves both the problems I guess.

[ndimiduk]

Exactly, these are the kinds of timelines I've been reasoning through.

All of these operations seem prone to data races because we have to reach back out to zookeeper to retrieve updated information. This is why I was designing a system that made use of the znode version information so that state updates at least always march forward, but that's apparently not available to us.

For what it's worth, what your building is essentially a Node Cache on each meta-region-server node, or maybe a Path Cache on a subset of the /hbase node. I'm not suggesting we introduce a dependency on Curator, but maybe that project has some useful ideas/examples/test facilities that would help here.

[bharathv]

Oh thanks for the pointers, didn't know about NodeCache and PatchCache. They'd definitely simplify the entire ZK state management in HBase.

[bharathv]

@ndimiduk Just publishing my thoughts on the main points of concern. I'm still thinking about how best to add test coverage for failure scenarios. Will update the patch tomorrow.

[bharathv]

I really wish the meta replica list was under a node of its own.

I was thinking the same.

Taking a step back, this whole logic is implemented to handle the following scenario. Consider a timeline of events (t n+1 > t n)

t1: standby master starts (populateInitialMetaLocations() runs in c'tor). No meta znodes, so nothing is populated in the cache
t2: Active master starts
t3: meta znodes created.

At this point standby's meta cache is stale and it hasn't registered any watchers, so nothing is notified and will remain a zombie forever.

So to fix this problem, populateInitialMetaLocations() registers a watcher on the base znode and also gets the meta znodes. So if the meta znodes turn out to be empty, we at least have a watcher on the base znode when something gets populated later. So the new timeline would be.

t1: standby master starts. No meta znodes, but registered a watcher on /hbase
t2: active master starts
t3: meta znodes created
t4: standby gets a notification that /hbase has changed and it populates the cache.

Do remember that we do not renew the watcher. So t4 is the last nodeChildrenChanged() event.

Now the only problem is in the following sequence of actions.

t1: active master starts
t2. meta znodes created.
t3: standby master starts, reads the meta znodes but also has a watcher on /hbase.

Now in the future if some children of /hbase changes, we have an extraneous invalidate of cache, but thats ok since that is the last time it is going to happen.

Now one may ask, why not do it the following way.

1. meta_znodes = getMetaZnodes();
2. if (meta znodes == empty) {
   register a watcher on /hbase
3. }

Unfortunately this is not atomic. Between (1) and (2), the meta znodes could be created and standby could remain stale for a while until some children of /hbase changes.

Sorry for the long post. Hope this explains my line of thought. Feel free to point anything that I'm missing here. :-)

[bharathv]

One additional thought, what if some other thread registers a watcher on the base znode and the cache thread processes them (guess this is possible). should we make populateInitialMetaLocations() idempotent? That solves both the problems I guess.

[bharathv]

Sorry I have to force push because I messed something up locally.

[bharathv]

Ok, I rebased against a wrong branch and that messed up the tree. Fixed it now.