|
[
Permlink
| « Hide
]
Tsz Wo (Nicholas), SZE added a comment - 18/Jul/08 10:38 PM
3169_20080718.patch: start the lease checker daemon when the first file is created, not in the DFSClient constructor.
3169_20080721.patch: added a test and some messages.
I feel that we do not need two locks, started and synchornized pendingCreates in LeaseChecker. Instead we can make put and remove synchronized.
I use two locks for protecting two objects, pendingCreate and daemon. You are right that it is possible to use one lock.
3169_20080723b.patch: only use one lock.
+1 for simple locking using LeaseChecker object instead of AtomicBoolean and a synchronizedSortedMap.
Also should the LeaseChecker thread be closed when there are no leases pending for a few seconds or so? The justification is same as the one for delaying creation of the thread. > Also should the LeaseChecker thread be closed when there are no leases pending for a few seconds or so? The justification is same as the one for delaying creation of the thread.
This is a good idea. Hairong also has suggested this before. This is useful if there are many clients (with different users) and all of them keep creating and closing files. It seems such case is not common. So we don't want to introduce complex codes to handle it. I agree this is not required. It is just an extension of what this jira already proposes to to to. An improvement over an improvement
> This is useful if there are many clients (with different users) and all of them keep creating and closing files. I don't think this requires multiple clients or users. Imagine a case where a task writes one file and closes the file system. In that case, lease checker will stay as long as the task (because of FS cache) though there is one using it. I did not mean to make it a requirement for this jira. Passed tests locally, try hudson.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12386746/3169_20080723b.patch  against trunk revision 679601. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2949/testReport/ This message is automatically generated. 3169_20080725.patch: updated with trunk.
Need to try hudson again
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12386907/3169_20080725.patch against trunk revision 679879. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/2954/testReport/ This message is automatically generated. I've just committed this. Thanks, Nicholas!
Integrated in Hadoop-trunk #581 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/581/
) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Major
LeaseChecker daemon should not be started in DFSClient constructor
