throw new Exception to avoid ugly NPE

[brettKK]

We have developed a static analysis tool NPEDetector to find some potential NPE. Our analysis shows that some callees may return null in corner case(e.g. node crash , IOException), some of their callers have !=null check but some do not have. In this issue we post a patch which can add !=null based on existed !=null check. For example:

ZkGrep#parseZkSnapshot:

  return retFiles;
} catch (Exception e) {
  LOG.error("fail to parse zkSnapshot: " + lastZkSnapshot, e);
}

return null;

So parseZkSnapshot will return null while IOException happens. but its caller ZkGrep#processCommandLineArgs have no null checker:

File[] lastZkSnapshot = parseZkSnapshot(zkDataDirs[1], byTime);

// lastZkSnapshot[1] is the parsed last snapshot by byTime
grepZkSnapshot(lastZkSnapshot[1], patterns);

We should terminate the process while lastZkSnapshot == null

[lujiefsi]

The Exception message should contains the original IOException ?

[zhan849]

yes.

Tooling is fine here as NPE is caught outside, and proper error message are printed out.

BTW, wrapping IOException using generic Exception will erase the proper semantics that IOException carries, which is not a good practice.

[lujiefsi]

why not we check the null in ZkGrep#processCommandLineArgs:
if (lastZkSnapshot == null){
LOG.error("fail to gunzip file" + zkDataDirs[1]);
System.exit(1);
}

@brettKK

[zhan849]

@lujiefsi might not be a good idea to only check lastZkSnapshot, because gunzip() is used in multiple places and you need to check all of them.

[lujiefsi]

+1 LGTM. these changes avoid duplicate exception, and also have enough log message for post-mortem debug

[lujiefsi]

hum, 0--->o???

[zhan849]

ok now it looks good to me. Can you pls squash your commits into 1 and I will contact with committers to help you merge

[brettKK]

@zhan849 thanks for your suggestion and I squash my commits into one commit.

[lujiefsi]

we should combine https://issues.apache.org/jira/browse/HELIX-701 with this pull request,

[brettKK]

@lujiefsi , https://issues.apache.org/jira/browse/HELIX-701 has been automatically associated with this PR.