[
https://issues.apache.org/jira/browse/HDFS-5138?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Aaron T. Myers updated HDFS-5138:
---------------------------------
Attachment: HDFS-5138.patch
Thanks a lot for the review, Todd. Here's an updated patch which should address
all of your feedback. Please have another look. Detailed responses below.
bq. thanks for the description in the above JIRA comment. Can you transfer this
comment somewhere into the docs, perhaps
hadoop-hdfs-project/hadoop-hdfs/src/site/apt/HDFSHighAvailabilityWithQJM.apt.vm
or a new page? Perhaps with a slightly more user-facing angle.
Good thinking. Done.
bq. what would happen if the admin called finalizeUpgrade() when neither node
had yet transitioned to active? I don't see any sanity check here.. is it
possible you'd end up leaving the shared in an orphaned "upgrading" state and
never end up finalizing it?
Great point. I've now added support for running finalizeUpgrade() on each NN
from DFSAdmin, and made the DFSAdmin command also check to ensure that one NN
is indeed active before it will finalize the upgrade.
bq. Similarly, what happens if you start one NN with -upgrade, and you start
the other one without -upgrade. It seems to me it should check for the upgrade
lock file in the shared dir and say "looks like an upgrade is in progress,
please start the SBN with -upgrade".
Good thinking. Fixed.
bq. there are a few TODOs in the code that probably need to be addressed -
nothing big, just a few things you may have missed.
Yes, as I said, this was a prelim patch to make sure folks were OK with the
direction. All of them are pretty trivial and have been addressed in the latest
patch, except for this one, which I'd love to hear your thoughts on:
{code}
+ // TODO(atm): What should we do here in the case of multiple shared edits
+ // dirs, which isn't actually possible at the moment?
{code}
My suggestion is that until we do actually support having multiple shared edits
dirs (perhaps never) that we should just assert that there is in fact only a
single one here.
bq. would be good to add Javadoc on the new methods, so that JM implementors
know what the upgrade process looks like. i.e what is pre-upgrade, etc?
Yep, good thinking. Done. Please have a look at the new javadocs.
bq. "Could not perform upgrade or more JournalNodes" error message has some
missing words in it.
Eagle-eye Lipcon. Good catch. Done.
bq. this line should be unreachable, right? maybe an
AssertionError("Unreachable code") would make more sense? Also this same
exception message is used down below in canRollBack which isn't quite right.
Good thinking. Done.
bq. when you upgrade the journal, I'd think you'd to copy over all the data
from the PersistentLongFiles into the new dir?
Good catch. Done. I've also updated the tests to ensure that this happens
during upgrade of JNs.
bq. worth considering a protobuf for the shared log lock, in case we want to
add other fields to it later (instead of the custom serialization you do now)
I thought about this, but concluded that with just two fields it probably
wasn't worth it. Do you feel strongly about this?
bq. need try...finally around the code where you write the shared log lock. On
the read side you're also forgetting to close it.
Good catch. Fixed.
bq. the creation of the shared log lock file is non-atomic... I'm worried that
we may hit the race in practice, since the AtomicFileOutputStream implies an
fsync, which means that between the exists() check and the rename to the lock
file, you may really have a decently long time window. Maybe we can use locking
code like Storage does? Feel free to punt to a follow-up.
Note that in the case the lock file is being created on a JN, that the creation
of the shared lock file is within a synchronized section. I agree that this
could still be an issue in the NFS case, but seems to me like we can reasonably
punt that to a later JIRA.
bq. can you add a doc on doUpgradeOfSharedLogOnTransitionToActive()?
Yep, done.
bq. why are some of the functions package-private and others are public?
That was an error. You're right that the one that was public can be
package-private. Fixed.
bq. make it an abstract class or give it a private constructor so it can't be
instantiated, since it's just static methods
Done. Made it abstract.
bq. brief javadocs would be nice for these methods, even though they're
straight refactors of existing code.
Sure. Added.
bq. in canRollBack(), you throw an exception if there is no shared log. That
doesn't seem right...
It actually is correct, since we only ever call this method in the event that
HA is enabled, so there really should be a shared log. In the event that HA is
not enabled, the upgrade of the edits dirs will be done directly on the storage
dirs, instead of going through the edit log upgrade code path. The same is true
FSEditLog#doPreUpgrade, doUpgrade, doFinalize, etc. I've changed the names of
these methods to have "OfSharedLog" at the end to make this clear.
bq. capitalization of "RollBack" vs "Rollback" is a little inconsistent. Looks
like Rollback is consistently used prior to this patch, so probably best to
stick with that.
I attempted to consistenly use "Rollback" (one word) when it's used as a noun,
and "RollBack" (two words) when it's used as a verb, which I believe to be
grammatically correct. Perhaps it's best to stick with just one capitalization
scheme for method/variable names, for consistency, even if it is technically
incorrect English. Do you have an opinion here?
bq. in the switch statement on the startup option, I think you should keep the
ROLLBACK case, but just have it throw AssertionError – just to make sure we
don't accidentally have some case where we're passing it there but shouldn't
be.GA
Good thinking. Done.
> Support HDFS upgrade in HA
> --------------------------
>
> Key: HDFS-5138
> URL: https://issues.apache.org/jira/browse/HDFS-5138
> Project: Hadoop HDFS
> Issue Type: Bug
> Affects Versions: 2.1.1-beta
> Reporter: Kihwal Lee
> Assignee: Aaron T. Myers
> Priority: Blocker
> Attachments: HDFS-5138.patch, HDFS-5138.patch, HDFS-5138.patch,
> HDFS-5138.patch
>
>
> With HA enabled, NN wo't start with "-upgrade". Since there has been a layout
> version change between 2.0.x and 2.1.x, starting NN in upgrade mode was
> necessary when deploying 2.1.x to an existing 2.0.x cluster. But the only way
> to get around this was to disable HA and upgrade.
> The NN and the cluster cannot be flipped back to HA until the upgrade is
> finalized. If HA is disabled only on NN for layout upgrade and HA is turned
> back on without involving DNs, things will work, but finaliizeUpgrade won't
> work (the NN is in HA and it cannot be in upgrade mode) and DN's upgrade
> snapshots won't get removed.
> We will need a different ways of doing layout upgrade and upgrade snapshot.
> I am marking this as a 2.1.1-beta blocker based on feedback from others. If
> there is a reasonable workaround that does not increase maintenance window
> greatly, we can lower its priority from blocker to critical.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
