[ https://issues.apache.org/jira/browse/HDFS-3597?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13417784#comment-13417784
]
Todd Lipcon commented on HDFS-3597:
-----------------------------------
A few style nits:
{code}
+ public boolean storageVersionMatches(StorageInfo si) throws IOException {
{code}
Can this be package-private?
----
{code}
+ boolean sameCluster(FSImage si) {
{code}
Rename to {{isSameCluster}}
----
- Nit: in several places you have the {{||}} or {{&&}} operators at the beginning
of a line of a multi-line condition. Our style in most of the code base is to put these at
the end of the prior line (even though {{validateStorageInfo}} was previously doing it at
the start of line)
----
{code}
void validateStorageInfo(FSImage si) throws IOException {
- if(layoutVersion != si.getStorage().layoutVersion
- || namespaceID != si.getStorage().namespaceID
- || cTime != si.getStorage().cTime
- || !clusterID.equals(si.getClusterID())
- || !blockpoolID.equals(si.getBlockPoolID())) {
+ if (!sameCluster(si)
+ || layoutVersion != si.getStorage().layoutVersion) {
{code}
In an earlier comment you mentioned not wanting to change the behavior of validateStorageInfo.
But I think here you lost the check on layoutVersion. Should this be {{!sameCluster(si) ||
!storageVersionMatches(si.getStorage())}} instead, to maintain the old behavior?
----
- In the new test case, add a reference to this JIRA - e.g {{Regression test for HDFS-3597}}.
So if someone is unsure about the logic, it's easy to understand why the test was put there.
- Style: rename {{versfiles}} to {{versionFiles}} for clarity.
- You can reduce the scope of a lot of your variables inside {{doIt}}
> SNN can fail to start on upgrade
> --------------------------------
>
> Key: HDFS-3597
> URL: https://issues.apache.org/jira/browse/HDFS-3597
> Project: Hadoop HDFS
> Issue Type: Bug
> Affects Versions: 2.0.0-alpha
> Reporter: Andy Isaacson
> Assignee: Andy Isaacson
> Priority: Minor
> Attachments: hdfs-3597-2.txt, hdfs-3597-3.txt, hdfs-3597.txt
>
>
> When upgrading from 1.x to 2.0.0, the SecondaryNameNode can fail to start up:
> {code}
> 2012-06-16 09:52:33,812 ERROR org.apache.hadoop.hdfs.server.namenode.SecondaryNameNode:
Exception in doCheckpoint
> java.io.IOException: Inconsistent checkpoint fields.
> LV = -40 namespaceID = 64415959 cTime = 1339813974990 ; clusterId = CID-07a82b97-8d04-4fdd-b3a1-f40650163245
; blockpoolId = BP-1792677198-172.29.121.67-1339813967723.
> Expecting respectively: -19; 64415959; 0; ; .
> at org.apache.hadoop.hdfs.server.namenode.CheckpointSignature.validateStorageInfo(CheckpointSignature.java:120)
> at org.apache.hadoop.hdfs.server.namenode.SecondaryNameNode.doCheckpoint(SecondaryNameNode.java:454)
> at org.apache.hadoop.hdfs.server.namenode.SecondaryNameNode.doWork(SecondaryNameNode.java:334)
> at org.apache.hadoop.hdfs.server.namenode.SecondaryNameNode$2.run(SecondaryNameNode.java:301)
> at org.apache.hadoop.security.SecurityUtil.doAsLoginUserOrFatal(SecurityUtil.java:438)
> at org.apache.hadoop.hdfs.server.namenode.SecondaryNameNode.run(SecondaryNameNode.java:297)
> at java.lang.Thread.run(Thread.java:662)
> {code}
> The error check we're hitting came from HDFS-1073, and it's intended to verify that we're
connecting to the correct NN. But the check is too strict and considers "different metadata
version" to be the same as "different clusterID".
> I believe the check in {{doCheckpoint}} simply needs to explicitly check for and handle
the update case.
--
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
|