[
https://issues.apache.org/jira/browse/HDFS-13286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16450226#comment-16450226
]
Erik Krogen commented on HDFS-13286:
------------------------------------
Great work Chao, this looks clean. I have quite a few comments on v001 but most
are very minor nits:
* DummyHAService L59, L272 / HAAdmin L76 are too long
* Description of {{-transitionToObserver}} should say "service into Observer"
instead of "service to Observer" for consistency
* Unnecessary line break in HAAdmin L243/244
* HAAdmin L278/280 should say "supports" as opposed to "support"
* There are a few whitespace changes that probably shouldn't be included in
this patch?
* I don't think the Javadoc for {{HAServiceProtocol#transitionToObserver}} with
{{ServiceFailedException}} is as it should be. This Javadoc refers to allowable
state transitions, which should actually be determined by the specific
implementation of the protocol (e.g. some service may allow transitions from
active to observer). I think it should just say "if transition from standby to
observer fails" and the implementation determines failure criteria.
* This block in {{NameNode}} seems wrong to me; the if-condition does not match
the comment. Either the comment should be updated to say "transition from
active to observer is forbidden" or the if-statement should be updated to {{if
(state != STANDBY_STATE)}}. I think I prefer the latter.
{code}
// Transition from state other than STANDBY to OBSERVER is forbidden.
if (state == ACTIVE_STATE) {
throw new ServiceFailedException(
"Cannot transition from Active to Observer");
}
{code}
* The method override in {{RMHAServiceTarget#supportObserver}} is unnecessary;
the default is already {{false}}
* The if-condition in {{StandbyState#setState}} is incorrect. If {{isObserver}}
is true and {{s == NameNode.OBSERVER_STATE}}, the if-block should not be
triggered, but currently it is.
* I guess this is from a previous JIRA, but I don't really understand why there
isn't a new subclass of {{StandbyState}} called {{ObserverState}}. The
{{isObserver}} flag seems messy and I think could be handled more cleanly via
method overrides of {{setState}} and {{toString}}.
* In {{TestDFSHAAdminMiniCluster#testObserverIllegalTransition}}, I think after
the transition attempts which are expected to fail, there should be asserts
verifying that in addition to returning -1, the state was not actually changed.
> Add haadmin commands to transition between standby and observer
> ---------------------------------------------------------------
>
> Key: HDFS-13286
> URL: https://issues.apache.org/jira/browse/HDFS-13286
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Chao Sun
> Assignee: Chao Sun
> Priority: Major
> Attachments: HDFS-13286-HDFS-12943.000.patch,
> HDFS-13286-HDFS-12943.001.patch
>
>
> As discussed in HDFS-12975, we should allow explicit transition between
> standby and observer through haadmin command, such as:
> {code}
> haadmin -transitionToObserver
> {code}
> Initially we should support transition from observer to standby, and standby
> to observer.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
