Ok I have to look into it. Do you have a patch available?
On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
[email protected]> wrote:
> One issue we found in CacheIndexInput.java which is causing NPE
>
> private CacheValue lookup(boolean quietly) {
>
> CacheValue cacheValue = _indexInputCache.get(_key.getBlockId());
>
> .......
>
> return cacheValue;
>
> //There is no eviction check for the CacheValue returned from
> IndexInputCache, causing NPE
>
> }
>
> Also, lookup method blindly adds to _indexInputCache before returning.
> Instead, it would be better if it is done inside the null-check loop...
>
> On Fri, Jul 22, 2016 at 6:09 PM, Ravikumar Govindarajan <
> [email protected]> wrote:
>
> > Thanks for the feedback Aaron
> >
> > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <[email protected]>
> wrote:
> >
> >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> >> [email protected]> wrote:
> >>
> >> > Just now saw BlockLocks code. It is documented to be thread-safe.
> >> Apologize
> >> > for the trouble...
> >> >
> >> > Btw, a small nit. The below method is not returning true. Is that
> >> > intentional?
> >> >
> >> > boolean releaseIfValid(long address) {
> >> >
> >> > if (address >= _address && address < _maxAddress) {
> >> >
> >> > long offset = address - _address;
> >> >
> >> > int index = (int) (offset / _chunkSize);
> >> >
> >> > _locks.clear(index);
> >> >
> >> > }
> >> >
> >> > return false;
> >> >
> >> > }
> >> >
> >>
> >> In my 30 second review I think you are right. It should probably return
> >> true. However I want to alanyze what happens with the current code so I
> >> can write a test that proves there is a problem (because there probably
> >> is)
> >> and fix it.
> >>
> >>
> >> >
> >> > Also, I thought a background thread can attempt merging sparsely
> >> populated
> >> > slabs into one single slab & release free-mem (in 128MB chunks) back
> to
> >> > OS...
> >> >
> >>
> >> I think this is a good idea, I just didn't get to writing it.
> >>
> >>
> >> >
> >> > You think it could be beneficial or it would make it needlessly
> complex?
> >> >
> >>
> >> I think for dedicated servers is might be overkill, but for a mixed
> >> workload environment (think docker containers and the like) it would be
> >> useful.
> >>
> >> Aaron
> >>
> >>
> >> >
> >> > On Wed, Jul 20, 2016 at 11:38 PM, Aaron McCurry <[email protected]>
> >> > wrote:
> >> >
> >> > > I don't think there is a race condition because the allocation
> occurs
> >> > > atomically in the BlockLocks class. Do see a problem? Let me know.
> >> > >
> >> > > Aaron
> >> > >
> >> > > On Wed, Jul 20, 2016 at 9:19 AM, Ravikumar Govindarajan <
> >> > > [email protected]> wrote:
> >> > >
> >> > > > I came across the following in
> >> SlabAllocationCacheValueBufferPool.java.
> >> > > Is
> >> > > > the below method thread-safe?
> >> > > >
> >> > > > @Override
> >> > > >
> >> > > > public CacheValue getCacheValue(int cacheBlockSize) {
> >> > > >
> >> > > > validCacheBlockSize(cacheBlockSize);
> >> > > >
> >> > > > int numberOfChunks = getNumberOfChunks(cacheBlockSize);
> >> > > >
> >> > > > ...
> >> > > >
> >> > > > }
> >> > > >
> >> > > >
> >> > > > It does allocation in a tight-loop using BlockLocks, Slab &
> Chunks.
> >> Is
> >> > > > there a race-condition where 2 threads can pick same slab & chunk?
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>
