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 < [email protected]> 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 <[email protected]> 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 < > > [email protected]> 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 <[email protected]> > > wrote: > > > > > > > Ok. Thanks! I will patch the code and run some tests. > > > > > > > > On Monday, August 1, 2016, Ravikumar Govindarajan < > > > > [email protected]> 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 <[email protected] > > > > > <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 < > > > > > > [email protected] <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 < > > > > > > > [email protected] <javascript:;>> wrote: > > > > > > > > > > > > > > > Thanks for the feedback Aaron > > > > > > > > > > > > > > > > On Thu, Jul 21, 2016 at 6:42 PM, Aaron McCurry < > > > [email protected] > > > > > <javascript:;>> > > > > > > > wrote: > > > > > > > > > > > > > > > >> On Thu, Jul 21, 2016 at 1:55 AM, Ravikumar Govindarajan < > > > > > > > >> [email protected] <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 < > > > > > [email protected] <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 > < > > > > > > > >> > > [email protected] <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? > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
