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" <rcm...@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 <ser...@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" <rcm...@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 <ser...@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
>>>>
>>>
>>>
>
