You left in a "TODO: this should be namespace-scoped". Also, is that referring to all of those config keys or just the immediate one below it? If you're not going to address this here, could you please file a new JIRA for it?
I don't understand the purpose of all the casting/wrapping of Exception/RuntimeException in ZKFailoverController#run. If this is really required, please add a comment as to why it's necessary.
Agreed, that was goofy. Fixed to always wrap/unwrap – reason being that the implemented interface doesn't throw exceptions, but our code does.
Could you please add a constant (and perhaps a more-likely-unique name) for the "wrapped" string used in ZKFailoverController#run?
Obsoleted by above fix.
"// TODO: need to hook DFS here to find the NN keytab info, etc," - do you intend to do this here? If not, please file a JIRA.
Will file a JIRA for auto-failover plus security - it needs a lot of manual testing, etc, so I wanted to do it as a follow-up.
If possible, let's make ElectorCallbacks and HealthCallbacks static, and have them take references to the ZKFailoverController instance at construction.
Tried to do this, but these inner classes are really just grouping of methods inside ZKFC. They access so many of the outer class's variables that it didn't seem any cleaner to make them static here.
Is there some reason you didn't use one of the option-parsing classes in ZKFailoverController#doRun?
The args are simple enough here I didn't see any benefit. And I've actually found commons-CLI to be a pain when there are "dependent" options (e.g -force only makes sense when -formatZK is passed)
Typo: "Gracefully uitting...".
Seems a little strange to me to output a log message about quitting the master election that will presumably appear in the logs before we've ever joined the election. I think this might confuse users.
That actually won't appear, since the HealthMonitor starts in INITIALIZING state. So, it doesn't fire a callback at startup, until it changes to something other than INITIALIZING. I will change the message to be something a little more general, though: "Ensuring that the local node does not participate in active election."
The design doc on
HDFS-2185 doesn't mention that the ZKFailoverController calls quitElection in the case of a transition to the INITIALIZING state. This seems like the right thing to do to me, but I just wanted to bring it to your attention.
Added to next rev of the design doc, thanks.
Seems a little odd to me to throw an AssertionError in the case of an unrecognized state transition. Perhaps an IllegalStateException would be better?
Changed to IllegalArgumentException – since from the perspective of ZK, the state is an argument of the callback, rather than its own state.
Left in the "TODO: we need to make sure that if we get fenced and then quickly restarted". What are the ramifications of not addressing this TODO initially?
I think it's pretty unlikely to happen in practice, since we set the IPC client retry parameter to 1, and it needs some very strange timing. But, for example, we have this bad scenario:
- ZKFC1 gets active lock
- ZKFC1 is about to send transitionToActive() and machine freezes (eg GC pause + swapping)
- ZKFC1 loses its ZK lock, ZKFC2 gets ZK lock
- ZKFC2 calls transitionToStandby on NN1, and transitions NN2 to active
- ZKFC1 wakes up from pause, calls transitionToActive(), now we have a bad situation
The timing is unlikely here, because we need the "freeze" on ZKFC1 to be longer than its own session timeout. I do think we need to fix it, but I want to think carefully about the solution, and it will require more "plumbing" to get it right. I'll file a follow-up JIRA when this is committed.
The method comment and the prompt in ToolRunner#confirmPrompt suggest that the user needs to enter a capital "Y", but the code in fact only accepts lower-case "y".
I think you misread it. It is case-insensitive. Clarified the javadoc to note this.
Might be a good idea to output a message along the lines of "Invalid input: 'x'" if the user provides an invalid input in ToolRunner#confirmPrompt.
Why the variable name "serial" in DummyHAService#getInstance? 'index' seems clearer to me.
I'd recommend you change the setting of DummyHAService#index to be before adding the object to the instances array, so that the index is zero-based. This will also let you take out the "- 1" from DummyHAService#getInstance.
That's actually what I had in the previous revision. But then I found it confusing that my variables were named "
" but the logs would refer to "service 0". I kept having cognitive dissonance when the logs told me something about "service 1" and I had to actually look at the code referring to svc2. Is that OK?
TestZKFailoverController does more than set up the configuration. Perhaps just "setUp" ?
I couldn't use "setUp" since the base class already has that. Changed to setupConfAndServices and renamed setupServicesAndFCs to just setupFCs which is more accurate.
I think it'd be better to make the ZKFailoverController error codes positive integers...
Changed to start them at 2 (didn't start with 1, since that's the error code that Java returns for uncaught exceptions coming out of main())
Please add a comment to TestZKFailoverController#setupServicesAndFCs that svc1 is guaranteed to initially be active. You also might want to assert that svc2 is initially STANDBY. Several of the tests assume this state up-front without actually asserting it.
"Allowing svc1 to be healthy again, making svc2 unhealthy..." - here you're actually making svc2 unreachable, not unhealthy.
The test code in TestZKFailoverController#testAutoFailoverOnLostZKSession is effectively repeated 4 times. Factor out?
In TestZKFailoverController#testDontFailoverToUnhealthyNode, let's assert after the Thread.sleep that svc2 is not in fact healthy. As it stands, the test could erroneously pass if svc2 became ACTIVE and then went back to STANDBY upon svc1 becoming healthy again.
Added a check that the lock is un-held after the 1-second sleep
I don't see any tests in this patch for the case of ZK completely disappearing. Please write one, or file a JIRA to do so.
Added a new test case testZooKeeperFailure()