harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [jira] Created: (HARMONY-6675) Reducing timeout value in CanonicalPatchCache to fix a file not found error in Hadoop common
Date Wed, 17 Nov 2010 20:05:00 GMT
On 17 November 2010 01:49, Regis <xu.regis@gmail.com> wrote:
> On 2010-11-16 18:40, sebb wrote:
>>
>> On 16 November 2010 06:56, Regis<xu.regis@gmail.com>  wrote:
>>>
>>> On 2010-11-11 13:26, Regis wrote:
>>>>
>>>> On 2010-11-11 13:13, Charles Lee wrote:
>>>>>
>>>>> The cache is good for the performance. But when the cache meet link,
>>>>> there
>>>>> will be some difficult situation. The patch has attached is a
>>>>> workaround for
>>>>> the specified test case. The good choose for the timeout should bed
>>>>> discussed.
>>>>>
>>>>> On Thu, Nov 11, 2010 at 7:30 AM, Guillermo Cabrera (JIRA)
>>>>> <jira@apache.org>wrote:
>>>>>
>>>>>> Reducing timeout value in CanonicalPatchCache to fix a file not found
>>>>>> error
>>>>>> in Hadoop common
>>>>>>
>>>>>>
>>>>>>
>>>>>> --------------------------------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>> Key: HARMONY-6675
>>>>>> URL: https://issues.apache.org/jira/browse/HARMONY-6675
>>>>>> Project: Harmony
>>>>>> Issue Type: New Feature
>>>>>> Environment: SLE v. 11, Apache Harmony 6
>>>>>> Reporter: Guillermo Cabrera
>>>>>> Priority: Minor
>>>>>>
>>>>>>
>>>>>> Testing Harmony Select (r1022137) with Hadoop common, we ran across
>>>>>> the
>>>>>> following error:
>>>>>>
>>>>>> java.lang.RuntimeException: Error while running command to get file
>>>>>> permissions : org.apache.hadoop.util.Shell$ExitCodeException: /bin/ls:
>>>>>> /tmp/test1/file: No such file or directory
>>>>>>
>>>>>> Charles Lee (Apache Harmony developer) provided us with the following
>>>>>> answer:
>>>>>>
>>>>>> "For all the test case failures in
>>>>>> org.apache.hadoop.fs.TestLocalFSFileContextSymlink, the root cause
is
>>>>>> we
>>>>>> have a CanonicalPathCache under the File, so the file canonical path
>>>>>> will be
>>>>>> wrong if the test case highly stressed, (I remember the cache time
is
>>>>>> set to
>>>>>> 10 minute)."
>>>>>>
>>>>>> His patch to fix this issue has been attached.
>>>>>>
>>>>>> --
>>>>>> This message is automatically generated by JIRA.
>>>>>> -
>>>>>> You can reply to this email to add a comment to the issue online.
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> I think we should provide a way to configure this cache: enable/disable
>>>> it and timeout, something like
>>>> -Dorg.apache.harmony.canonpath.cache.timeout=60, this property could be
>>>> overwritten at run time.
>>>>
>>>> Also the default value of timeout, 10 minutes seems too long, maybe
>>>> reduce to one minute or half minute is reasonable for the most of
>>>> applications?
>>>>
>>>
>>> I propose following patch, which configure canonical path cache via
>>> system
>>> property 'org.apache.harmony.file.canonical.path.cache.timeout', set it
>>> to
>>> zero could disable the cache completely. And I also reduce default
>>> timeout
>>> from 10 minutes to 30 seconds. If no one object, I'll commit this path
>>> soon.
>>>
>>>
>>> Index:
>>>
>>> modules/luni/src/main/java/org/apache/harmony/luni/internal/io/FileCanonPathCache.java
>>> =====================================================================
>>> ---
>>>
>>> modules/luni/src/main/java/org/apache/harmony/luni/internal/io/FileCanonPathCache.java
>>> +++
>>>
>>> modules/luni/src/main/java/org/apache/harmony/luni/internal/io/FileCanonPathCache.java
>>> @@ -55,9 +55,29 @@ public class FileCanonPathCache {
>>>     private static Object lock = new Object();
>>>
>>>     /**
>>> -     * Expired time.
>>> +     * Expired time, 0 disable this cache.
>>>      */
>>> -    private static long timeout = 600000;
>>> +    private static long timeout = 30000;
>>
>> Static variables should really be final (even if private).
>
> This variable can't be 'final' because we provide a method setTimeout()
> which set a new value of 'timeout'.

Just had a look at the code.

Effectively the timeout field is a public mutable static variable.
Access to the field is not synchronised at all.

So if one thread sets a new value, other threads may or may not see
the updated value (ever).
Worse, unless the JVM guarantees that writes to long variables are
atomic, AFAICT if two threads write the value, then it's possible that
the timeout field could end up with a value that is equal to neither
setting.

Making the field volatile should fix these problems - I think.

There is a subtle bug - if the property sets the timeout to <=0, then
isEnable is set to false. setTimeout() never sets it true.

Furthermore, since access to isEnable is not synch. either, if one
thread sets timeout 0 and the another to non-zero, isValid could end
up being incorrect for the value of the timeout. It's always risky
when two fields are interdependent like this.

It would be safer to eliminate the isValid field and just check the
value of the timeout.

BTW, the private instance fields in CacheElement should be final; that
would make the class immutable and thread-safe.

>>
>>> +
>>> +    /**
>>> +     * Whether to enable this cache.
>>> +     */
>>> +    private static boolean isEnable = true;
>>> +
>>
>> Likewise.
>>
>>> +    public static final String FILE_CANONICAL_PATH_CACHE_TIMEOUT =
>>> "org.apache.harmony.file.canonical.path.cache.timeout";
>>> +
>>> +    static {
>>> +        String value =
>>> System.getProperty(FILE_CANONICAL_PATH_CACHE_TIMEOUT);
>>> +        try {
>>> +            timeout = Long.parseLong(value);
>>> +        } catch (NumberFormatException e) {
>>> +            // use default timeout value
>>> +        }
>>> +
>>> +        if (timeout<= 0) {
>>> +            isEnable = false;
>>> +        }
>>> +    }
>>>
>>>     /**
>>>      * Retrieve element from cache.
>>> @@ -68,6 +88,10 @@ public class FileCanonPathCache {
>>>      *
>>>      */
>>>     public static String get(String path) {
>>> +        if (!isEnable) {
>>> +            return null;
>>> +        }
>>> +
>>>         CacheElement element = null;
>>>         synchronized (lock) {
>>>             element = cache.get(path);
>>> @@ -104,6 +128,10 @@ public class FileCanonPathCache {
>>>      *            the canonical path of<code>path</code>.
>>>      */
>>>     public static void put(String path, String canonicalPath) {
>>> +        if (!isEnable) {
>>> +            return;
>>> +        }
>>> +
>>>         CacheElement element = new CacheElement(canonicalPath);
>>>         synchronized (lock) {
>>>             if (cache.size()>= CACHE_SIZE) {
>>> @@ -120,6 +148,10 @@ public class FileCanonPathCache {
>>>      * Remove all elements from cache.
>>>      */
>>>     public static void clear() {
>>> +        if (!isEnable) {
>>> +            return;
>>> +        }
>>> +
>>>         synchronized (lock) {
>>>             cache.clear();
>>>             list.clear();
>>> @@ -132,5 +164,8 @@ public class FileCanonPathCache {
>>>
>>>     public static void setTimeout(long timeout) {
>>>         FileCanonPathCache.timeout = timeout;
>>> +        if (timeout<= 0) {
>>> +            isEnable = false;
>>> +        }
>>>     }
>>>  }
>>>
>>>
>>> --
>>> Best Regards,
>>> Regis.
>>>
>>
>
>
> --
> Best Regards,
> Regis.
>

Mime
View raw message