Tsz Wo Nicholas Sze
added a comment - 12/Aug/09 18:32 > No bug fix and no change of semantics ...
That's great. I was not sure which section in CHANGES.txt should this belongs to when I tried to commit this yesterday.
I have committed this. Thanks, Kan.

> What are the changes about? Is there any bug fix?
No bug fix and no change of semantics to DFSClient and DataXceiver. 2 package private methods are added to DFSClient to read access token for testing purposes. The rest of the changes are simply changes to logging messages.

Kan Zhang
added a comment - 12/Aug/09 18:03 > What are the changes about? Is there any bug fix?
No bug fix and no change of semantics to DFSClient and DataXceiver. 2 package private methods are added to DFSClient to read access token for testing purposes. The rest of the changes are simply changes to logging messages.

Kan Zhang
added a comment - 11/Aug/09 22:35 > I am not sure about TestNameNodeMetrics.
I took a look and as far as I can tell, it was unrelated. I also ran the test a few times on my local box, they all passed.

Tsz Wo Nicholas Sze
added a comment - 11/Aug/09 22:27 Many tests failed in Hudson but most of them are related to HDFS-534 and HADOOP-6188 but not this issue. I am not sure about TestNameNodeMetrics. Could you check it, Kan?

ran hdfs unit tests and all passed except one test (TestHDFSTrash), which was unrelated. Here is the test-patch result.

[exec] +1 overall. [exec][exec] +1 @author. The patch does not contain any @author tags.[exec][exec] +1 tests included. The patch appears to include 15 new or modified tests.[exec][exec] +1 javadoc. The javadoc tool did not generate any warning messages.[exec][exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.[exec][exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.[exec][exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

Kan Zhang
added a comment - 11/Aug/09 02:28 ran hdfs unit tests and all passed except one test (TestHDFSTrash), which was unrelated. Here is the test-patch result.
[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 15 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.

Looks much better. The separate shouldWait method really improves the code readability, thanks for that. My only remaining concern is that within the sleep section, an Exception is caught rather than an InterruptedException, which will swallow any problems that occur. Once this is corrected, it should be good to go.

Jakob Homan
added a comment - 11/Aug/09 01:08 Looks much better. The separate shouldWait method really improves the code readability, thanks for that. My only remaining concern is that within the sleep section, an Exception is caught rather than an InterruptedException , which will swallow any problems that occur. Once this is corrected, it should be good to go.

uploading a new patch with added javadoc. I didn't break waitActive() into 2 loops since that would add one more RPC call. Instead, I refactored the wait logic into a separate method, which should improve readability and also get rid of any redundant checking.

Kan Zhang
added a comment - 08/Aug/09 02:08 uploading a new patch with added javadoc. I didn't break waitActive() into 2 loops since that would add one more RPC call. Instead, I refactored the wait logic into a separate method, which should improve readability and also get rid of any redundant checking.

I think the changes to MiniDFSCluster::waitActive could be improved by creating two loops, one for dn registration and one for dn heartbeats. You'd have the same number of RPC calls, but improve the number of times through the loops, as well as making the code easier to understand.

Otherwise looks good. I really like the heavy documentation in the unit tests. Looks faithful to the test plan.

Jakob Homan
added a comment - 07/Aug/09 19:29 Reviewed.
restartDataNode(DataNodeProperties dnprop, boolean keepPort) needs javadoc
I think the changes to MiniDFSCluster::waitActive could be improved by creating two loops, one for dn registration and one for dn heartbeats. You'd have the same number of RPC calls, but improve the number of times through the loops, as well as making the code easier to understand.
Otherwise looks good. I really like the heavy documentation in the unit tests. Looks faithful to the test plan.