You are viewing a plain text version of this content. The canonical link for it is here.
Posted to blur-user@incubator.apache.org by Ravikumar Govindarajan <ra...@gmail.com> on 2016/07/20 13:19:30 UTC
SlabAllocationCacheValueBufferPool thread-safe?
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?
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
I will. Thanks!
On Wed, Sep 7, 2016 at 6:37 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> wrote:
> Created JIRA issue. Please do go through it. Not sure if community will
> find this useful
> https://issues.apache.org/jira/browse/BLUR-481
>
> On Mon, Sep 5, 2016 at 9:50 PM, Aaron McCurry <am...@gmail.com> wrote:
>
> > Hey Ravi. Not sure if you attached the patch on email or not. The mail
> > system removes attachments. You can open an issue here:
> >
> > https://issues.apache.org/jira/browse/BLUR
> >
> > Or post it to a gist or if it really is small you can just include in an
> > email.
> >
> > Thanks!
> >
> > Aaron
> >
> > On Tue, Aug 30, 2016 at 1:44 AM, Ravikumar Govindarajan <
> > ravikumar.govindarajan@gmail.com> wrote:
> >
> > > PF the small patch we made for this. We had a specific requirement to
> > > solve. Don't know how useful it will be for the community!
> > >
> > >
> > >
> > > On Thu, Aug 25, 2016 at 11:40 AM, Ravikumar Govindarajan <
> > > ravikumar.govindarajan@gmail.com> wrote:
> > >
> > >> Sure, will share the patch in a couple of days...
> > >>
> > >> On Tue, Aug 23, 2016 at 9:18 PM, Aaron McCurry <am...@gmail.com>
> > >> wrote:
> > >>
> > >>> Cool, sounds good. If you could send me the changes you have made
> that
> > >>> would be great. It would make it easier to integrate your changes
> back
> > >>> into the project. Thanks!
> > >>>
> > >>> Aaron
> > >>>
> > >>> On Wed, Aug 17, 2016 at 8:40 AM, Ravikumar Govindarajan <
> > >>> ravikumar.govindarajan@gmail.com> wrote:
> > >>>
> > >>> > I removed _cacheValueQuietRefCannotBeReleased & instead directly
> > used
> > >>> a
> > >>> > ByteArrayCacheValue every-time fillQuietly() is called.
> > >>> >
> > >>> > Now searches seem to work correctly. Not sure if it's because of
> > >>> clone() or
> > >>> > may be something else...
> > >>> >
> > >>> > FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go
> easy
> > >>> on gc
> > >>> >
> > >>> > On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <amccurry@gmail.com
> >
> > >>> wrote:
> > >>> >
> > >>> > > I'm not sure why that IOE is happening but if you want to change
> > the
> > >>> > quiet
> > >>> > > behavior this is where you can control it. There's a config and
> an
> > >>> > object
> > >>> > > there to change the behavior.
> > >>> > >
> > >>> > > https://github.com/apache/incubator-blur/blob/master/
> > >>> > > blur-store/src/main/java/org/apache/blur/store/
> > >>> > > BlockCacheDirectoryFactoryV2.java#L171
> > >>> > >
> > >>> > > On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
> > >>> > > ravikumar.govindarajan@gmail.com> wrote:
> > >>> > >
> > >>> > > > Aaron,
> > >>> > > >
> > >>> > > > Just one more help...
> > >>> > > >
> > >>> > > > I hardcoded _quiet=true in CacheIndexInput.java and started the
> > >>> > > > shard-server. It seems to mangle the cached-bytes & results in
> > >>> > > IOException
> > >>> > > > during searches. Merges however work smoothly...
> > >>> > > >
> > >>> > > > I do know that _quiet is meant only for merge. But there is a
> > >>> use-case
> > >>> > I
> > >>> > > am
> > >>> > > > working on, which will need this setting during searches
> also...
> > >>> > > >
> > >>> > > > Any quick suggestions for this issue?
> > >>> > > >
> > >>> > > > --
> > >>> > > > Ravi
> > >>> > > >
> > >>> > > > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <
> > amccurry@gmail.com>
> > >>> > > wrote:
> > >>> > > >
> > >>> > > > > Ok. Thanks! I will patch the code and run some tests.
> > >>> > > > >
> > >>> > > > > On Monday, August 1, 2016, Ravikumar Govindarajan <
> > >>> > > > > ravikumar.govindarajan@gmail.com> wrote:
> > >>> > > > >
> > >>> > > > > > We did a simple expiry check. Works fine as of now...
> > >>> > > > > >
> > >>> > > > > > private CacheValue lookup(boolean quietly) {
> > >>> > > > > >
> > >>> > > > > > CacheValue cacheValue = _indexInputCache.get(_key.
> > >>> > getBlockId());
> > >>> > > > > >
> > >>> > > > > > if(cacheValue == null || *cacheValue.isExpired()*) {
> > >>> > > > > >
> > >>> > > > > > ....
> > >>> > > > > >
> > >>> > > > > > }
> > >>> > > > > >
> > >>> > > > > > }
> > >>> > > > > >
> > >>> > > > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <
> > >>> amccurry@gmail.com
> > >>> > > > > > <javascript:;>> wrote:
> > >>> > > > > >
> > >>> > > > > > > Ok I have to look into it. Do you have a patch
> available?
> > >>> > > > > > >
> > >>> > > > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > >>> > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > >>> > > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>>
> wrote:
> > >>> > > > > > > >
> > >>> > > > > > > > > Thanks for the feedback Aaron
> > >>> > > > > > > > >
> > >>> > > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
> > >>> > > > amccurry@gmail.com
> > >>> > > > > > <javascript:;>>
> > >>> > > > > > > > wrote:
> > >>> > > > > > > > >
> > >>> > > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar
> > Govindarajan
> > >>> <
> > >>> > > > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>>
> > 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 <
> > >>> > > > > > amccurry@gmail.com <javascript:;>
> > >>> > > > > > > >
> > >>> > > > > > > > >> > 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
> > >>> > <
> > >>> > > > > > > > >> > > ravikumar.govindarajan@gmail.com
> <javascript:;>>
> > >>> 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?
> > >>> > > > > > > > >> > > >
> > >>> > > > > > > > >> > >
> > >>> > > > > > > > >> >
> > >>> > > > > > > > >>
> > >>> > > > > > > > >
> > >>> > > > > > > > >
> > >>> > > > > > > >
> > >>> > > > > > >
> > >>> > > > > >
> > >>> > > > >
> > >>> > > >
> > >>> > >
> > >>> >
> > >>>
> > >>
> > >>
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
Created JIRA issue. Please do go through it. Not sure if community will
find this useful
https://issues.apache.org/jira/browse/BLUR-481
On Mon, Sep 5, 2016 at 9:50 PM, Aaron McCurry <am...@gmail.com> wrote:
> Hey Ravi. Not sure if you attached the patch on email or not. The mail
> system removes attachments. You can open an issue here:
>
> https://issues.apache.org/jira/browse/BLUR
>
> Or post it to a gist or if it really is small you can just include in an
> email.
>
> Thanks!
>
> Aaron
>
> On Tue, Aug 30, 2016 at 1:44 AM, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> wrote:
>
> > PF the small patch we made for this. We had a specific requirement to
> > solve. Don't know how useful it will be for the community!
> >
> >
> >
> > On Thu, Aug 25, 2016 at 11:40 AM, Ravikumar Govindarajan <
> > ravikumar.govindarajan@gmail.com> wrote:
> >
> >> Sure, will share the patch in a couple of days...
> >>
> >> On Tue, Aug 23, 2016 at 9:18 PM, Aaron McCurry <am...@gmail.com>
> >> wrote:
> >>
> >>> Cool, sounds good. If you could send me the changes you have made that
> >>> would be great. It would make it easier to integrate your changes back
> >>> into the project. Thanks!
> >>>
> >>> Aaron
> >>>
> >>> On Wed, Aug 17, 2016 at 8:40 AM, Ravikumar Govindarajan <
> >>> ravikumar.govindarajan@gmail.com> wrote:
> >>>
> >>> > I removed _cacheValueQuietRefCannotBeReleased & instead directly
> used
> >>> a
> >>> > ByteArrayCacheValue every-time fillQuietly() is called.
> >>> >
> >>> > Now searches seem to work correctly. Not sure if it's because of
> >>> clone() or
> >>> > may be something else...
> >>> >
> >>> > FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go easy
> >>> on gc
> >>> >
> >>> > On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <am...@gmail.com>
> >>> wrote:
> >>> >
> >>> > > I'm not sure why that IOE is happening but if you want to change
> the
> >>> > quiet
> >>> > > behavior this is where you can control it. There's a config and an
> >>> > object
> >>> > > there to change the behavior.
> >>> > >
> >>> > > https://github.com/apache/incubator-blur/blob/master/
> >>> > > blur-store/src/main/java/org/apache/blur/store/
> >>> > > BlockCacheDirectoryFactoryV2.java#L171
> >>> > >
> >>> > > On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
> >>> > > ravikumar.govindarajan@gmail.com> wrote:
> >>> > >
> >>> > > > Aaron,
> >>> > > >
> >>> > > > Just one more help...
> >>> > > >
> >>> > > > I hardcoded _quiet=true in CacheIndexInput.java and started the
> >>> > > > shard-server. It seems to mangle the cached-bytes & results in
> >>> > > IOException
> >>> > > > during searches. Merges however work smoothly...
> >>> > > >
> >>> > > > I do know that _quiet is meant only for merge. But there is a
> >>> use-case
> >>> > I
> >>> > > am
> >>> > > > working on, which will need this setting during searches also...
> >>> > > >
> >>> > > > Any quick suggestions for this issue?
> >>> > > >
> >>> > > > --
> >>> > > > Ravi
> >>> > > >
> >>> > > > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <
> amccurry@gmail.com>
> >>> > > wrote:
> >>> > > >
> >>> > > > > Ok. Thanks! I will patch the code and run some tests.
> >>> > > > >
> >>> > > > > On Monday, August 1, 2016, Ravikumar Govindarajan <
> >>> > > > > ravikumar.govindarajan@gmail.com> wrote:
> >>> > > > >
> >>> > > > > > We did a simple expiry check. Works fine as of now...
> >>> > > > > >
> >>> > > > > > private CacheValue lookup(boolean quietly) {
> >>> > > > > >
> >>> > > > > > CacheValue cacheValue = _indexInputCache.get(_key.
> >>> > getBlockId());
> >>> > > > > >
> >>> > > > > > if(cacheValue == null || *cacheValue.isExpired()*) {
> >>> > > > > >
> >>> > > > > > ....
> >>> > > > > >
> >>> > > > > > }
> >>> > > > > >
> >>> > > > > > }
> >>> > > > > >
> >>> > > > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <
> >>> amccurry@gmail.com
> >>> > > > > > <javascript:;>> wrote:
> >>> > > > > >
> >>> > > > > > > Ok I have to look into it. Do you have a patch available?
> >>> > > > > > >
> >>> > > > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> >>> > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> >>> > > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> >>> > > > > > > >
> >>> > > > > > > > > Thanks for the feedback Aaron
> >>> > > > > > > > >
> >>> > > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
> >>> > > > amccurry@gmail.com
> >>> > > > > > <javascript:;>>
> >>> > > > > > > > wrote:
> >>> > > > > > > > >
> >>> > > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar
> Govindarajan
> >>> <
> >>> > > > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>>
> 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 <
> >>> > > > > > amccurry@gmail.com <javascript:;>
> >>> > > > > > > >
> >>> > > > > > > > >> > 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
> >>> > <
> >>> > > > > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>>
> >>> 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?
> >>> > > > > > > > >> > > >
> >>> > > > > > > > >> > >
> >>> > > > > > > > >> >
> >>> > > > > > > > >>
> >>> > > > > > > > >
> >>> > > > > > > > >
> >>> > > > > > > >
> >>> > > > > > >
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> >>
> >>
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
Hey Ravi. Not sure if you attached the patch on email or not. The mail
system removes attachments. You can open an issue here:
https://issues.apache.org/jira/browse/BLUR
Or post it to a gist or if it really is small you can just include in an
email.
Thanks!
Aaron
On Tue, Aug 30, 2016 at 1:44 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> wrote:
> PF the small patch we made for this. We had a specific requirement to
> solve. Don't know how useful it will be for the community!
>
>
>
> On Thu, Aug 25, 2016 at 11:40 AM, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> wrote:
>
>> Sure, will share the patch in a couple of days...
>>
>> On Tue, Aug 23, 2016 at 9:18 PM, Aaron McCurry <am...@gmail.com>
>> wrote:
>>
>>> Cool, sounds good. If you could send me the changes you have made that
>>> would be great. It would make it easier to integrate your changes back
>>> into the project. Thanks!
>>>
>>> Aaron
>>>
>>> On Wed, Aug 17, 2016 at 8:40 AM, Ravikumar Govindarajan <
>>> ravikumar.govindarajan@gmail.com> wrote:
>>>
>>> > I removed _cacheValueQuietRefCannotBeReleased & instead directly used
>>> a
>>> > ByteArrayCacheValue every-time fillQuietly() is called.
>>> >
>>> > Now searches seem to work correctly. Not sure if it's because of
>>> clone() or
>>> > may be something else...
>>> >
>>> > FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go easy
>>> on gc
>>> >
>>> > On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <am...@gmail.com>
>>> wrote:
>>> >
>>> > > I'm not sure why that IOE is happening but if you want to change the
>>> > quiet
>>> > > behavior this is where you can control it. There's a config and an
>>> > object
>>> > > there to change the behavior.
>>> > >
>>> > > https://github.com/apache/incubator-blur/blob/master/
>>> > > blur-store/src/main/java/org/apache/blur/store/
>>> > > BlockCacheDirectoryFactoryV2.java#L171
>>> > >
>>> > > On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
>>> > > ravikumar.govindarajan@gmail.com> wrote:
>>> > >
>>> > > > Aaron,
>>> > > >
>>> > > > Just one more help...
>>> > > >
>>> > > > I hardcoded _quiet=true in CacheIndexInput.java and started the
>>> > > > shard-server. It seems to mangle the cached-bytes & results in
>>> > > IOException
>>> > > > during searches. Merges however work smoothly...
>>> > > >
>>> > > > I do know that _quiet is meant only for merge. But there is a
>>> use-case
>>> > I
>>> > > am
>>> > > > working on, which will need this setting during searches also...
>>> > > >
>>> > > > Any quick suggestions for this issue?
>>> > > >
>>> > > > --
>>> > > > Ravi
>>> > > >
>>> > > > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com>
>>> > > wrote:
>>> > > >
>>> > > > > Ok. Thanks! I will patch the code and run some tests.
>>> > > > >
>>> > > > > On Monday, August 1, 2016, Ravikumar Govindarajan <
>>> > > > > ravikumar.govindarajan@gmail.com> wrote:
>>> > > > >
>>> > > > > > We did a simple expiry check. Works fine as of now...
>>> > > > > >
>>> > > > > > private CacheValue lookup(boolean quietly) {
>>> > > > > >
>>> > > > > > CacheValue cacheValue = _indexInputCache.get(_key.
>>> > getBlockId());
>>> > > > > >
>>> > > > > > if(cacheValue == null || *cacheValue.isExpired()*) {
>>> > > > > >
>>> > > > > > ....
>>> > > > > >
>>> > > > > > }
>>> > > > > >
>>> > > > > > }
>>> > > > > >
>>> > > > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <
>>> amccurry@gmail.com
>>> > > > > > <javascript:;>> wrote:
>>> > > > > >
>>> > > > > > > Ok I have to look into it. Do you have a patch available?
>>> > > > > > >
>>> > > > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
>>> > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
>>> > > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
>>> > > > > > > >
>>> > > > > > > > > Thanks for the feedback Aaron
>>> > > > > > > > >
>>> > > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
>>> > > > amccurry@gmail.com
>>> > > > > > <javascript:;>>
>>> > > > > > > > wrote:
>>> > > > > > > > >
>>> > > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan
>>> <
>>> > > > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
>>> > > > > > amccurry@gmail.com <javascript:;>
>>> > > > > > > >
>>> > > > > > > > >> > 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
>>> > <
>>> > > > > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>>
>>> 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?
>>> > > > > > > > >> > > >
>>> > > > > > > > >> > >
>>> > > > > > > > >> >
>>> > > > > > > > >>
>>> > > > > > > > >
>>> > > > > > > > >
>>> > > > > > > >
>>> > > > > > >
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>>
>>
>>
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
PF the small patch we made for this. We had a specific requirement to
solve. Don't know how useful it will be for the community!
On Thu, Aug 25, 2016 at 11:40 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> wrote:
> Sure, will share the patch in a couple of days...
>
> On Tue, Aug 23, 2016 at 9:18 PM, Aaron McCurry <am...@gmail.com> wrote:
>
>> Cool, sounds good. If you could send me the changes you have made that
>> would be great. It would make it easier to integrate your changes back
>> into the project. Thanks!
>>
>> Aaron
>>
>> On Wed, Aug 17, 2016 at 8:40 AM, Ravikumar Govindarajan <
>> ravikumar.govindarajan@gmail.com> wrote:
>>
>> > I removed _cacheValueQuietRefCannotBeReleased & instead directly used a
>> > ByteArrayCacheValue every-time fillQuietly() is called.
>> >
>> > Now searches seem to work correctly. Not sure if it's because of
>> clone() or
>> > may be something else...
>> >
>> > FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go easy on
>> gc
>> >
>> > On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <am...@gmail.com>
>> wrote:
>> >
>> > > I'm not sure why that IOE is happening but if you want to change the
>> > quiet
>> > > behavior this is where you can control it. There's a config and an
>> > object
>> > > there to change the behavior.
>> > >
>> > > https://github.com/apache/incubator-blur/blob/master/
>> > > blur-store/src/main/java/org/apache/blur/store/
>> > > BlockCacheDirectoryFactoryV2.java#L171
>> > >
>> > > On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
>> > > ravikumar.govindarajan@gmail.com> wrote:
>> > >
>> > > > Aaron,
>> > > >
>> > > > Just one more help...
>> > > >
>> > > > I hardcoded _quiet=true in CacheIndexInput.java and started the
>> > > > shard-server. It seems to mangle the cached-bytes & results in
>> > > IOException
>> > > > during searches. Merges however work smoothly...
>> > > >
>> > > > I do know that _quiet is meant only for merge. But there is a
>> use-case
>> > I
>> > > am
>> > > > working on, which will need this setting during searches also...
>> > > >
>> > > > Any quick suggestions for this issue?
>> > > >
>> > > > --
>> > > > Ravi
>> > > >
>> > > > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com>
>> > > wrote:
>> > > >
>> > > > > Ok. Thanks! I will patch the code and run some tests.
>> > > > >
>> > > > > On Monday, August 1, 2016, Ravikumar Govindarajan <
>> > > > > ravikumar.govindarajan@gmail.com> wrote:
>> > > > >
>> > > > > > We did a simple expiry check. Works fine as of now...
>> > > > > >
>> > > > > > private CacheValue lookup(boolean quietly) {
>> > > > > >
>> > > > > > CacheValue cacheValue = _indexInputCache.get(_key.
>> > getBlockId());
>> > > > > >
>> > > > > > if(cacheValue == null || *cacheValue.isExpired()*) {
>> > > > > >
>> > > > > > ....
>> > > > > >
>> > > > > > }
>> > > > > >
>> > > > > > }
>> > > > > >
>> > > > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <
>> amccurry@gmail.com
>> > > > > > <javascript:;>> wrote:
>> > > > > >
>> > > > > > > Ok I have to look into it. Do you have a patch available?
>> > > > > > >
>> > > > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
>> > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
>> > > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
>> > > > > > > >
>> > > > > > > > > Thanks for the feedback Aaron
>> > > > > > > > >
>> > > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
>> > > > amccurry@gmail.com
>> > > > > > <javascript:;>>
>> > > > > > > > wrote:
>> > > > > > > > >
>> > > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
>> > > > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
>> > > > > > amccurry@gmail.com <javascript:;>
>> > > > > > > >
>> > > > > > > > >> > 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
>> > <
>> > > > > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>>
>> 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?
>> > > > > > > > >> > > >
>> > > > > > > > >> > >
>> > > > > > > > >> >
>> > > > > > > > >>
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
Sure, will share the patch in a couple of days...
On Tue, Aug 23, 2016 at 9:18 PM, Aaron McCurry <am...@gmail.com> wrote:
> Cool, sounds good. If you could send me the changes you have made that
> would be great. It would make it easier to integrate your changes back
> into the project. Thanks!
>
> Aaron
>
> On Wed, Aug 17, 2016 at 8:40 AM, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> wrote:
>
> > I removed _cacheValueQuietRefCannotBeReleased & instead directly used a
> > ByteArrayCacheValue every-time fillQuietly() is called.
> >
> > Now searches seem to work correctly. Not sure if it's because of clone()
> or
> > may be something else...
> >
> > FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go easy on
> gc
> >
> > On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <am...@gmail.com>
> wrote:
> >
> > > I'm not sure why that IOE is happening but if you want to change the
> > quiet
> > > behavior this is where you can control it. There's a config and an
> > object
> > > there to change the behavior.
> > >
> > > https://github.com/apache/incubator-blur/blob/master/
> > > blur-store/src/main/java/org/apache/blur/store/
> > > BlockCacheDirectoryFactoryV2.java#L171
> > >
> > > On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
> > > ravikumar.govindarajan@gmail.com> wrote:
> > >
> > > > Aaron,
> > > >
> > > > Just one more help...
> > > >
> > > > I hardcoded _quiet=true in CacheIndexInput.java and started the
> > > > shard-server. It seems to mangle the cached-bytes & results in
> > > IOException
> > > > during searches. Merges however work smoothly...
> > > >
> > > > I do know that _quiet is meant only for merge. But there is a
> use-case
> > I
> > > am
> > > > working on, which will need this setting during searches also...
> > > >
> > > > Any quick suggestions for this issue?
> > > >
> > > > --
> > > > Ravi
> > > >
> > > > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com>
> > > wrote:
> > > >
> > > > > Ok. Thanks! I will patch the code and run some tests.
> > > > >
> > > > > On Monday, August 1, 2016, Ravikumar Govindarajan <
> > > > > ravikumar.govindarajan@gmail.com> wrote:
> > > > >
> > > > > > We did a simple expiry check. Works fine as of now...
> > > > > >
> > > > > > private CacheValue lookup(boolean quietly) {
> > > > > >
> > > > > > CacheValue cacheValue = _indexInputCache.get(_key.
> > getBlockId());
> > > > > >
> > > > > > if(cacheValue == null || *cacheValue.isExpired()*) {
> > > > > >
> > > > > > ....
> > > > > >
> > > > > > }
> > > > > >
> > > > > > }
> > > > > >
> > > > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <
> amccurry@gmail.com
> > > > > > <javascript:;>> wrote:
> > > > > >
> > > > > > > Ok I have to look into it. Do you have a patch available?
> > > > > > >
> > > > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> > > > > > > >
> > > > > > > > > Thanks for the feedback Aaron
> > > > > > > > >
> > > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
> > > > amccurry@gmail.com
> > > > > > <javascript:;>>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > > > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > > > amccurry@gmail.com <javascript:;>
> > > > > > > >
> > > > > > > > >> > 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
> > <
> > > > > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>>
> 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?
> > > > > > > > >> > > >
> > > > > > > > >> > >
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
Cool, sounds good. If you could send me the changes you have made that
would be great. It would make it easier to integrate your changes back
into the project. Thanks!
Aaron
On Wed, Aug 17, 2016 at 8:40 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> wrote:
> I removed _cacheValueQuietRefCannotBeReleased & instead directly used a
> ByteArrayCacheValue every-time fillQuietly() is called.
>
> Now searches seem to work correctly. Not sure if it's because of clone() or
> may be something else...
>
> FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go easy on gc
>
> On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <am...@gmail.com> wrote:
>
> > I'm not sure why that IOE is happening but if you want to change the
> quiet
> > behavior this is where you can control it. There's a config and an
> object
> > there to change the behavior.
> >
> > https://github.com/apache/incubator-blur/blob/master/
> > blur-store/src/main/java/org/apache/blur/store/
> > BlockCacheDirectoryFactoryV2.java#L171
> >
> > On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
> > ravikumar.govindarajan@gmail.com> wrote:
> >
> > > Aaron,
> > >
> > > Just one more help...
> > >
> > > I hardcoded _quiet=true in CacheIndexInput.java and started the
> > > shard-server. It seems to mangle the cached-bytes & results in
> > IOException
> > > during searches. Merges however work smoothly...
> > >
> > > I do know that _quiet is meant only for merge. But there is a use-case
> I
> > am
> > > working on, which will need this setting during searches also...
> > >
> > > Any quick suggestions for this issue?
> > >
> > > --
> > > Ravi
> > >
> > > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com>
> > wrote:
> > >
> > > > Ok. Thanks! I will patch the code and run some tests.
> > > >
> > > > On Monday, August 1, 2016, Ravikumar Govindarajan <
> > > > ravikumar.govindarajan@gmail.com> wrote:
> > > >
> > > > > We did a simple expiry check. Works fine as of now...
> > > > >
> > > > > private CacheValue lookup(boolean quietly) {
> > > > >
> > > > > CacheValue cacheValue = _indexInputCache.get(_key.
> getBlockId());
> > > > >
> > > > > if(cacheValue == null || *cacheValue.isExpired()*) {
> > > > >
> > > > > ....
> > > > >
> > > > > }
> > > > >
> > > > > }
> > > > >
> > > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <amccurry@gmail.com
> > > > > <javascript:;>> wrote:
> > > > >
> > > > > > Ok I have to look into it. Do you have a patch available?
> > > > > >
> > > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> > > > > > >
> > > > > > > > Thanks for the feedback Aaron
> > > > > > > >
> > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
> > > amccurry@gmail.com
> > > > > <javascript:;>>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > > amccurry@gmail.com <javascript:;>
> > > > > > >
> > > > > > > >> > 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
> <
> > > > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>> 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?
> > > > > > > >> > > >
> > > > > > > >> > >
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
I removed _cacheValueQuietRefCannotBeReleased & instead directly used a
ByteArrayCacheValue every-time fillQuietly() is called.
Now searches seem to work correctly. Not sure if it's because of clone() or
may be something else...
FYI, I modified ByteArrayCacheValue to use a Store-Buffer to go easy on gc
On Wed, Aug 10, 2016 at 8:12 PM, Aaron McCurry <am...@gmail.com> wrote:
> I'm not sure why that IOE is happening but if you want to change the quiet
> behavior this is where you can control it. There's a config and an object
> there to change the behavior.
>
> https://github.com/apache/incubator-blur/blob/master/
> blur-store/src/main/java/org/apache/blur/store/
> BlockCacheDirectoryFactoryV2.java#L171
>
> On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> wrote:
>
> > Aaron,
> >
> > Just one more help...
> >
> > I hardcoded _quiet=true in CacheIndexInput.java and started the
> > shard-server. It seems to mangle the cached-bytes & results in
> IOException
> > during searches. Merges however work smoothly...
> >
> > I do know that _quiet is meant only for merge. But there is a use-case I
> am
> > working on, which will need this setting during searches also...
> >
> > Any quick suggestions for this issue?
> >
> > --
> > Ravi
> >
> > On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com>
> wrote:
> >
> > > Ok. Thanks! I will patch the code and run some tests.
> > >
> > > On Monday, August 1, 2016, Ravikumar Govindarajan <
> > > ravikumar.govindarajan@gmail.com> wrote:
> > >
> > > > We did a simple expiry check. Works fine as of now...
> > > >
> > > > private CacheValue lookup(boolean quietly) {
> > > >
> > > > CacheValue cacheValue = _indexInputCache.get(_key.getBlockId());
> > > >
> > > > if(cacheValue == null || *cacheValue.isExpired()*) {
> > > >
> > > > ....
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <amccurry@gmail.com
> > > > <javascript:;>> wrote:
> > > >
> > > > > Ok I have to look into it. Do you have a patch available?
> > > > >
> > > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> > > > > >
> > > > > > > Thanks for the feedback Aaron
> > > > > > >
> > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
> > amccurry@gmail.com
> > > > <javascript:;>>
> > > > > > wrote:
> > > > > > >
> > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > amccurry@gmail.com <javascript:;>
> > > > > >
> > > > > > >> > 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 <
> > > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>> 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?
> > > > > > >> > > >
> > > > > > >> > >
> > > > > > >> >
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
I'm not sure why that IOE is happening but if you want to change the quiet
behavior this is where you can control it. There's a config and an object
there to change the behavior.
https://github.com/apache/incubator-blur/blob/master/blur-store/src/main/java/org/apache/blur/store/BlockCacheDirectoryFactoryV2.java#L171
On Wed, Aug 10, 2016 at 10:37 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> wrote:
> Aaron,
>
> Just one more help...
>
> I hardcoded _quiet=true in CacheIndexInput.java and started the
> shard-server. It seems to mangle the cached-bytes & results in IOException
> during searches. Merges however work smoothly...
>
> I do know that _quiet is meant only for merge. But there is a use-case I am
> working on, which will need this setting during searches also...
>
> Any quick suggestions for this issue?
>
> --
> Ravi
>
> On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com> wrote:
>
> > Ok. Thanks! I will patch the code and run some tests.
> >
> > On Monday, August 1, 2016, Ravikumar Govindarajan <
> > ravikumar.govindarajan@gmail.com> wrote:
> >
> > > We did a simple expiry check. Works fine as of now...
> > >
> > > private CacheValue lookup(boolean quietly) {
> > >
> > > CacheValue cacheValue = _indexInputCache.get(_key.getBlockId());
> > >
> > > if(cacheValue == null || *cacheValue.isExpired()*) {
> > >
> > > ....
> > >
> > > }
> > >
> > > }
> > >
> > > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <amccurry@gmail.com
> > > <javascript:;>> wrote:
> > >
> > > > Ok I have to look into it. Do you have a patch available?
> > > >
> > > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> > > > >
> > > > > > Thanks for the feedback Aaron
> > > > > >
> > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <
> amccurry@gmail.com
> > > <javascript:;>>
> > > > > wrote:
> > > > > >
> > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > amccurry@gmail.com <javascript:;>
> > > > >
> > > > > >> > 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 <
> > > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>> 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?
> > > > > >> > > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
Aaron,
Just one more help...
I hardcoded _quiet=true in CacheIndexInput.java and started the
shard-server. It seems to mangle the cached-bytes & results in IOException
during searches. Merges however work smoothly...
I do know that _quiet is meant only for merge. But there is a use-case I am
working on, which will need this setting during searches also...
Any quick suggestions for this issue?
--
Ravi
On Thu, Aug 4, 2016 at 2:43 PM, Aaron McCurry <am...@gmail.com> wrote:
> Ok. Thanks! I will patch the code and run some tests.
>
> On Monday, August 1, 2016, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> wrote:
>
> > We did a simple expiry check. Works fine as of now...
> >
> > private CacheValue lookup(boolean quietly) {
> >
> > CacheValue cacheValue = _indexInputCache.get(_key.getBlockId());
> >
> > if(cacheValue == null || *cacheValue.isExpired()*) {
> >
> > ....
> >
> > }
> >
> > }
> >
> > On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <amccurry@gmail.com
> > <javascript:;>> wrote:
> >
> > > Ok I have to look into it. Do you have a patch available?
> > >
> > > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> > > >
> > > > > Thanks for the feedback Aaron
> > > > >
> > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <amccurry@gmail.com
> > <javascript:;>>
> > > > wrote:
> > > > >
> > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > amccurry@gmail.com <javascript:;>
> > > >
> > > > >> > 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 <
> > > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>> 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?
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
Ok. Thanks! I will patch the code and run some tests.
On Monday, August 1, 2016, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> wrote:
> We did a simple expiry check. Works fine as of now...
>
> private CacheValue lookup(boolean quietly) {
>
> CacheValue cacheValue = _indexInputCache.get(_key.getBlockId());
>
> if(cacheValue == null || *cacheValue.isExpired()*) {
>
> ....
>
> }
>
> }
>
> On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <amccurry@gmail.com
> <javascript:;>> wrote:
>
> > Ok I have to look into it. Do you have a patch available?
> >
> > On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> > ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> > > ravikumar.govindarajan@gmail.com <javascript:;>> wrote:
> > >
> > > > Thanks for the feedback Aaron
> > > >
> > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <amccurry@gmail.com
> <javascript:;>>
> > > wrote:
> > > >
> > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > > >> ravikumar.govindarajan@gmail.com <javascript:;>> 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 <
> amccurry@gmail.com <javascript:;>
> > >
> > > >> > 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 <
> > > >> > > ravikumar.govindarajan@gmail.com <javascript:;>> 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?
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
We did a simple expiry check. Works fine as of now...
private CacheValue lookup(boolean quietly) {
CacheValue cacheValue = _indexInputCache.get(_key.getBlockId());
if(cacheValue == null || *cacheValue.isExpired()*) {
....
}
}
On Sun, Jul 31, 2016 at 2:14 AM, Aaron McCurry <am...@gmail.com> wrote:
> Ok I have to look into it. Do you have a patch available?
>
> On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> 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 <
> > ravikumar.govindarajan@gmail.com> wrote:
> >
> > > Thanks for the feedback Aaron
> > >
> > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <am...@gmail.com>
> > wrote:
> > >
> > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> > >> ravikumar.govindarajan@gmail.com> 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 <amccurry@gmail.com
> >
> > >> > 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 <
> > >> > > ravikumar.govindarajan@gmail.com> 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?
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
Ok I have to look into it. Do you have a patch available?
On Thu, Jul 28, 2016 at 4:47 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> 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 <
> ravikumar.govindarajan@gmail.com> wrote:
>
> > Thanks for the feedback Aaron
> >
> > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <am...@gmail.com>
> wrote:
> >
> >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> >> ravikumar.govindarajan@gmail.com> 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 <am...@gmail.com>
> >> > 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 <
> >> > > ravikumar.govindarajan@gmail.com> 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?
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
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 <
ravikumar.govindarajan@gmail.com> wrote:
> Thanks for the feedback Aaron
>
> On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <am...@gmail.com> wrote:
>
>> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
>> ravikumar.govindarajan@gmail.com> 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 <am...@gmail.com>
>> > 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 <
>> > > ravikumar.govindarajan@gmail.com> 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?
>> > > >
>> > >
>> >
>>
>
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
Thanks for the feedback Aaron
On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry <am...@gmail.com> wrote:
> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
> ravikumar.govindarajan@gmail.com> 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 <am...@gmail.com>
> > 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 <
> > > ravikumar.govindarajan@gmail.com> 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?
> > > >
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan <
ravikumar.govindarajan@gmail.com> 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 <am...@gmail.com>
> 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 <
> > ravikumar.govindarajan@gmail.com> 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?
> > >
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Ravikumar Govindarajan <ra...@gmail.com>.
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;
}
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...
You think it could be beneficial or it would make it needlessly complex?
On Wed, Jul 20, 2016 at 11:38 PM, Aaron McCurry <am...@gmail.com> 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 <
> ravikumar.govindarajan@gmail.com> 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?
> >
>
Re: SlabAllocationCacheValueBufferPool thread-safe?
Posted by Aaron McCurry <am...@gmail.com>.
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 <
ravikumar.govindarajan@gmail.com> 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?
>