incubator-blur-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ravikumar Govindarajan <ravikumar.govindara...@gmail.com>
Subject Re: SlabAllocationCacheValueBufferPool thread-safe?
Date Wed, 17 Aug 2016 12:40:56 GMT
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?
> > > > > > >> > > >
> > > > > > >> > >
> > > > > > >> >
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message