[
https://issues.apache.org/jira/browse/HADOOP-8206?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Todd Lipcon updated HADOOP-8206:
--------------------------------
Attachment: hadoop-8206.txt
bq. 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?
Will file.
bq. 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.
bq. 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.
bq. "// 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.
bq. 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.
bq. 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)
bq. Typo: "Gracefully uitting...".
Fixed
bq. 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."
bq. 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.
bq. 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.
bq. 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.
bq. 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.
bq. 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.
Fixed
bq. Why the variable name "serial" in DummyHAService#getInstance? 'index' seems
clearer to me.
bq. 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 "{thr,svc,zkfc}{1,2}" 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?
bq. 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.
bq. 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())
bq. 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.
Fixed.
bq. "Allowing svc1 to be healthy again, making svc2 unhealthy..." - here you're
actually making svc2 unreachable, not unhealthy.
Fixed
bq. The test code in TestZKFailoverController#testAutoFailoverOnLostZKSession
is effectively repeated 4 times. Factor out?
Fixed
bq. 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
bq. 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()
> Common portion of ZK-based failover controller
> ----------------------------------------------
>
> Key: HADOOP-8206
> URL: https://issues.apache.org/jira/browse/HADOOP-8206
> Project: Hadoop Common
> Issue Type: New Feature
> Components: ha
> Affects Versions: 0.24.0
> Reporter: Todd Lipcon
> Assignee: Todd Lipcon
> Attachments: hadoop-8206.txt, hadoop-8206.txt, hadoop-8206.txt
>
>
> This JIRA is for the Common (generic) portion of HDFS-2185. It can't run on
> its own, but this JIRA will include unit tests using mock/dummy services.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
