MockDirWrapper always syncs

[Shai Erera <se...@gmail.com>]

Hi

Look at this code from MDW.sync:

    if (true || LuceneTestCase.rarely(randomState) || delegate instanceof
NRTCachingDirectory) {
      // don't wear out our hardware so much in tests.
      delegate.sync(names);
    }

Is the 'if (true)' intentional or left there by mistake? The comment
afterwards suggests it should not be there, but I wanted to confirm before
I remove it.

Shai

Re: MockDirWrapper always syncs

[Simon Willnauer <si...@gmail.com>]

seems like unintentional... +1 to remove

On Wed, May 8, 2013 at 8:16 PM, Shai Erera <se...@gmail.com> wrote:
> Hi
>
> Look at this code from MDW.sync:
>
>     if (true || LuceneTestCase.rarely(randomState) || delegate instanceof
> NRTCachingDirectory) {
>       // don't wear out our hardware so much in tests.
>       delegate.sync(names);
>     }
>
> Is the 'if (true)' intentional or left there by mistake? The comment
> afterwards suggests it should not be there, but I wanted to confirm before I
> remove it.
>
> Shai

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

Re: MockDirWrapper always syncs

[Shai Erera <se...@gmail.com>]

Ok I will look at this more tomorrow. Perhaps we can start with a simple
test on RateLimitDir wrapping NRTCachingDir, before we add an interface?
Anyway I'll look at both options.

As for failing during sync, I suggested half following how copyBytes works,
as it's simple. Determining on a per file basis is also good. I'll open an
issue to handle both.

Shai
On May 8, 2013 11:03 PM, "Robert Muir" <rc...@gmail.com> wrote:

> Its unfortunately there until we get a chance to address it. I think the
> comment explains it correctly:
>
>     // TODO: need to improve hack to be OK w/
>     // RateLimitingDirWrapper in between...
>
> Actually I think Mark miller added some apis (getDelegate()) to wrapping
> directories like NRTCachingDirectory and RateLimiting. Maybe this should
> actually be pulled into a proper interface (DelegatingDirectory or
> something), and MockDirectoryWrapper (which should also implement this i
> suppose), could traverse the hierarchy and determine if any
> NRTCachingDirectories exist. Then we wouldnt have to actually call fsync so
> much in tests.
>
> As far as syncing only some files, I think its fair for MDW to iterate the
> collection of files and call sync on each one, with the possibility of a
> random exception happening so only some are synced. It sorta does this
> kinda thing with readBytes() etc already.
>
> On Wed, May 8, 2013 at 3:56 PM, Shai Erera <se...@gmail.com> wrote:
>
>> So you say this true is there for a purpose? Is that what the TODO above
>> refers to?
>>
>> If so, perhaps we should add some comment which better explains it.
>>
>> Also separately, currently sync either fails or succeeds to sync all
>> files. Do you think perhaps it could randomly choose to throw IOE after
>> syncing half the files?
>>
>> Shai
>> On May 8, 2013 10:48 PM, "Robert Muir" <rc...@gmail.com> wrote:
>>
>>> originally the if (true) did not exist.
>>>
>>> problem is (apart from system crash: where it matters to any directory),
>>> NRTCachingDirectory's sync() actually causes stuff to go to disk. otherwise
>>> it might just be sitting in RAM.
>>>
>>> however, then we added RateLimitingDirectory. so now you have
>>> possibility of RateLimitingDirectory(NRTCaching(.... and other structures.
>>> So its not easily possible to only sync sometimes without breaking things.
>>>
>>> On Wed, May 8, 2013 at 2:16 PM, Shai Erera <se...@gmail.com> wrote:
>>>
>>>> Hi
>>>>
>>>> Look at this code from MDW.sync:
>>>>
>>>>     if (true || LuceneTestCase.rarely(randomState) || delegate
>>>> instanceof NRTCachingDirectory) {
>>>>       // don't wear out our hardware so much in tests.
>>>>       delegate.sync(names);
>>>>     }
>>>>
>>>> Is the 'if (true)' intentional or left there by mistake? The comment
>>>> afterwards suggests it should not be there, but I wanted to confirm before
>>>> I remove it.
>>>>
>>>> Shai
>>>>
>>>
>>>
>

Re: MockDirWrapper always syncs

[Robert Muir <rc...@gmail.com>]

Its unfortunately there until we get a chance to address it. I think the
comment explains it correctly:

    // TODO: need to improve hack to be OK w/
    // RateLimitingDirWrapper in between...

Actually I think Mark miller added some apis (getDelegate()) to wrapping
directories like NRTCachingDirectory and RateLimiting. Maybe this should
actually be pulled into a proper interface (DelegatingDirectory or
something), and MockDirectoryWrapper (which should also implement this i
suppose), could traverse the hierarchy and determine if any
NRTCachingDirectories exist. Then we wouldnt have to actually call fsync so
much in tests.

As far as syncing only some files, I think its fair for MDW to iterate the
collection of files and call sync on each one, with the possibility of a
random exception happening so only some are synced. It sorta does this
kinda thing with readBytes() etc already.

On Wed, May 8, 2013 at 3:56 PM, Shai Erera <se...@gmail.com> wrote:

> So you say this true is there for a purpose? Is that what the TODO above
> refers to?
>
> If so, perhaps we should add some comment which better explains it.
>
> Also separately, currently sync either fails or succeeds to sync all
> files. Do you think perhaps it could randomly choose to throw IOE after
> syncing half the files?
>
> Shai
> On May 8, 2013 10:48 PM, "Robert Muir" <rc...@gmail.com> wrote:
>
>> originally the if (true) did not exist.
>>
>> problem is (apart from system crash: where it matters to any directory),
>> NRTCachingDirectory's sync() actually causes stuff to go to disk. otherwise
>> it might just be sitting in RAM.
>>
>> however, then we added RateLimitingDirectory. so now you have possibility
>> of RateLimitingDirectory(NRTCaching(.... and other structures. So its not
>> easily possible to only sync sometimes without breaking things.
>>
>> On Wed, May 8, 2013 at 2:16 PM, Shai Erera <se...@gmail.com> wrote:
>>
>>> Hi
>>>
>>> Look at this code from MDW.sync:
>>>
>>>     if (true || LuceneTestCase.rarely(randomState) || delegate
>>> instanceof NRTCachingDirectory) {
>>>       // don't wear out our hardware so much in tests.
>>>       delegate.sync(names);
>>>     }
>>>
>>> Is the 'if (true)' intentional or left there by mistake? The comment
>>> afterwards suggests it should not be there, but I wanted to confirm before
>>> I remove it.
>>>
>>> Shai
>>>
>>
>>

Re: MockDirWrapper always syncs

[Shai Erera <se...@gmail.com>]

So you say this true is there for a purpose? Is that what the TODO above
refers to?

If so, perhaps we should add some comment which better explains it.

Also separately, currently sync either fails or succeeds to sync all files.
Do you think perhaps it could randomly choose to throw IOE after syncing
half the files?

Shai
On May 8, 2013 10:48 PM, "Robert Muir" <rc...@gmail.com> wrote:

> originally the if (true) did not exist.
>
> problem is (apart from system crash: where it matters to any directory),
> NRTCachingDirectory's sync() actually causes stuff to go to disk. otherwise
> it might just be sitting in RAM.
>
> however, then we added RateLimitingDirectory. so now you have possibility
> of RateLimitingDirectory(NRTCaching(.... and other structures. So its not
> easily possible to only sync sometimes without breaking things.
>
> On Wed, May 8, 2013 at 2:16 PM, Shai Erera <se...@gmail.com> wrote:
>
>> Hi
>>
>> Look at this code from MDW.sync:
>>
>>     if (true || LuceneTestCase.rarely(randomState) || delegate instanceof
>> NRTCachingDirectory) {
>>       // don't wear out our hardware so much in tests.
>>       delegate.sync(names);
>>     }
>>
>> Is the 'if (true)' intentional or left there by mistake? The comment
>> afterwards suggests it should not be there, but I wanted to confirm before
>> I remove it.
>>
>> Shai
>>
>
>

Re: MockDirWrapper always syncs

[Robert Muir <rc...@gmail.com>]

originally the if (true) did not exist.

problem is (apart from system crash: where it matters to any directory),
NRTCachingDirectory's sync() actually causes stuff to go to disk. otherwise
it might just be sitting in RAM.

however, then we added RateLimitingDirectory. so now you have possibility
of RateLimitingDirectory(NRTCaching(.... and other structures. So its not
easily possible to only sync sometimes without breaking things.

On Wed, May 8, 2013 at 2:16 PM, Shai Erera <se...@gmail.com> wrote:

> Hi
>
> Look at this code from MDW.sync:
>
>     if (true || LuceneTestCase.rarely(randomState) || delegate instanceof
> NRTCachingDirectory) {
>       // don't wear out our hardware so much in tests.
>       delegate.sync(names);
>     }
>
> Is the 'if (true)' intentional or left there by mistake? The comment
> afterwards suggests it should not be there, but I wanted to confirm before
> I remove it.
>
> Shai
>