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 Tue, 30 Aug 2016 05:44:43 GMT
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 <amccurry@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?
>> > > > > > > > >> > > >
>> > > > > > > > >> > >
>> > > > > > > > >> >
>> > > > > > > > >>
>> > > > > > > > >
>> > > > > > > > >
>> > > > > > > >
>> > > > > > >
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>

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