I think this should take StorageInfo as a parameter instead, and you would pass image.getStorage() in.
Sounds good, thanks.
I'm not 100% convinced of the logic. I think we should always verify that it's the same NN – but just loosen the validateStorageInfo check here to not check the versioning info. For example, if I accidentally point my 2NN at the wrong NN, it won't start, even if that NN happens to be from a different version. It should only blow its local storage away if it's the same NN (namespace/cluster) but a different version.
Fair enough, but we don't want to loosen the check in validateStorageInfo because it's used in a half dozen other places that want full checking I think. I'll refactor the checks.
Instead, can you use FSImageTestUtil.corruptVersionFile here?
Great, didn't know about that!
No need for these...?
indeed, leftover from a previous test design.
Can you change this test to not need any datanodes? ... mkdir
A fine plan, done.
It seems odd that you print out all of the checkpoint dirs, but then only corrupt the property in one of them. Shouldn't you be corrupting it in all of them?
That's an issue I was confused about too. I don't understand why the test has multiple checkpoint dirs, nor why my 2NN is running in snn.getCheckpointDirs().get(1) rather than .get(0). (If I corrupt the first checkpointdir, there is no perceptible effect on the testcase.) The println is a leftover from when I was still attempting to exercise the upgrade code.
The spelling fix in NNStorage is unrelated. Cleanup's good, but try not to do so in files that aren't otherwise touched by your patch.
Dropped. At some point during development my fix touched NNstorage.