[
https://issues.apache.org/jira/browse/HDFS-13286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16450344#comment-16450344
]
Chao Sun commented on HDFS-13286:
---------------------------------
Thanks [~xkrogen] for the great comments!
bq. DummyHAService L59, L272 / HAAdmin L76 are too long
Fixed. Some of them I just followed the previous code (e.g.,
{{transitionToStandby}}): should I fix the other code too?
bq. Description of -transitionToObserver should say "service into Observer"
instead of "service to Observer" for consistency.
Okay... I'd prefer "into" for consistency too. [~ajayydv]: what's your opinion?
bq. Unnecessary line break in HAAdmin L243/244
Done.
bq. HAAdmin L278/280 should say "supports" as opposed to "support"
Done.
bq. There are a few whitespace changes that probably shouldn't be included in
this patch?
Yes noticed. These are trailing whitespace from previous commits, and for some
reason my IDE will automatically eliminate them. Let me add them back...
bq. 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.
Good point. I'll change it to just "if transition from standby to observer
fails".
bq. 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.
With the latter it needs to be changed to {{if (state != STANDBY_STATE && state
!= OBSERVER_STATE)}}, which is more verbose though. I'm thinking just to update
the comment. What do you think?
bq. The method override in RMHAServiceTarget#supportObserver is unnecessary;
the default is already false
Done.
bq. 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.
Good spot. Will fix and add a test for this.
bq. 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.
This is originally what I implemented but we eventually decided to go with the
flag approach, to eliminate duplicated code. Now it seems having a separate
{{ObserverState}} will make the logic clearer, and is less error prone as one
may easily treat a observer state as standby without paying much attention to
the flag. [~shv]: what do you think if we add back the {{ObserverState}}?
bq. 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.
Will add verification for these.
> 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]
