> On Sept. 27, 2016, 3:33 p.m., Anoop Sam John wrote: > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java, > > line 95 > > <https://reviews.apache.org/r/51785/diff/6/?file=1510155#file1510155line95> > > > > This is kind of merging all the segent's MemstoreLABs into one and then > > pass that to the result segment. Can have a such a method and in the > > ineterface? > > Anastasia Braginsky wrote: > Interface of what? MSLAB? I think MSLAB interface is going to be > refactored significantly with your offHeapMSLAB, let's do it all there...
Ya to MSLAB interface itself. Because we will have other impls also tomorow. Ok we can do that later. I can help with that. Drop the comment > On Sept. 27, 2016, 3:33 p.m., Anoop Sam John wrote: > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java, > > line 398 > > <https://reviews.apache.org/r/51785/diff/6/?file=1510146#file1510146line398> > > > > Seeing the tests using this method.. Why those tests can not create a > > new instance of CompactingMemstore (After setting the required conf and > > passing that)? Give this kind of setters if not aviodable. I feel we can > > do the test with out this also.. Pls consider. In Memstorecompactor > > constructor just initailize the action type. > > Anastasia Braginsky wrote: > In the current version of the tests it is impossible to create a new > instance of CompactingMemstore after setting the required conf. This whould > be something like this: > > memstore.getConfiguration().set("hbase.hregion.compacting.memstore.type", > "data-compaction"); > this.memstore = new CompactingMemStore(HBaseConfiguration.create(), > CellComparator.COMPARATOR, store, regionServicesForStores); > > This is problematic because the configuration is created per memstore. > This can be improved, but I don't want to do it under this JIRA. I think all > those Compacting MemStore tests need to be refined and new JIRA need to > opened for that. This JIRA is opened for too long and already includes too > many code lines. Let's finish with it. Ok can do later > On Sept. 27, 2016, 3:33 p.m., Anoop Sam John wrote: > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java, > > line 206 > > <https://reviews.apache.org/r/51785/diff/6/?file=1510149#file1510149line206> > > > > I can think of one issue here. Say we have MSLAB usage and when first > > time the cell was added to active segment, the copy to MSLAB happened. > > Means Cell 'c' data is in some MSLAB chunk. Now we copy to again to new > > area as have data merge. As part of this compaction, at the end, we will > > close the src segments which gets compacted. Means the chunks those > > segments holding, we will be releasing. (I assume we use pool always). > > Now consider this call of maybeCloneWithAllocator failed. Ya it can > > fail because at this point of time a free chunk and/or slot is not > > available. So it will just return the same Cell. And we will include that > > cell only in the result Segment. But we have closed the src segments and > > releases the chunks where this cell data recides. So we will get corrupt > > data! > > Anastasia Braginsky wrote: > I understand your point. If maybeCloneWithAllocator() method doesn't > really copies the cell to another the chunk, indeed the code is wrong. Now, > looking inside maybeCloneWithAllocator() I see that it can decide not to copy > in two cases: > > 1. We do not work with the MSLAB (this.memStoreLAB == null). I assume > this is not the case. > 2. The allocation of the bytes in the chunk fails > (this.memStoreLAB.allocateBytes(len) returns null). But in turn this can > happen only if the requested size is bigger then the maximal allowed > allocation size. Which is also not the case as we take the cell from MSLAB > and this is valid there. > > Bottom line, your concern is not real at least with the > maybeCloneWithAllocator() code that we have now. If you wonder about any > future implementation of maybeCloneWithAllocator(), then lets discuss it > later after this JIRA is commited and the future code is reviewed. Oh ya when we dont have enough chunks in pool, we will create new.. I just missed that point. Agree. NP. > On Sept. 27, 2016, 3:33 p.m., Anoop Sam John wrote: > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java, > > line 36 > > <https://reviews.apache.org/r/51785/diff/6/?file=1510154#file1510154line36> > > > > The 2 impls of this have diff wrt Scanner creation only no? In case of > > merge we dont need SQM and can use max long as read point > > In case of compaction we use SQM and smallest read pnt. > > Other than this, the hasNext() and next() can be same logic no? Only > > we can have createScanner as abstarct method and impl in both places? > > Anastasia Braginsky wrote: > The difference between MemStoreMergerSegmentsIterator and > MemStoreCompactorSegmentsIterator is that MemStoreCompactorSegmentsIterator > implements StoreScanner (compactingScanner) on top of the MemStoreScanner > (scanner) and MemStoreMergerSegmentsIterator uses the MemStoreScanner > (scanner) directly. The hasNext() and next() of > MemStoreCompactorSegmentsIterator need to keep managing of the KVS (as old > MemStoreCompactorIterator did) and hasNext() and next() of > MemStoreMergerSegmentsIterator just use MemStoreScanner directly. > > Do you say that MemStoreMergerSegmentsIterator should also build > StoreScanner on top of the MemStoreScanner and just not to use SQM? But why > that additional code is needed? Any possibility for refactor, can do later. Am ok. > On Sept. 27, 2016, 3:33 p.m., Anoop Sam John wrote: > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java, > > line 97 > > <https://reviews.apache.org/r/51785/diff/6/?file=1510155#file1510155line97> > > > > This typecasting is ok as of now. But when we have diff kind of MSLAB > > impl will be an issue. (OffheapMSLAB) > > Anastasia Braginsky wrote: > I prefer to deal with it all then... Ok. I can help u do that. - Anoop Sam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51785/#review150430 ----------------------------------------------------------- On Sept. 26, 2016, 8:57 p.m., Anastasia Braginsky wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51785/ > ----------------------------------------------------------- > > (Updated Sept. 26, 2016, 8:57 p.m.) > > > Review request for hbase. > > > Repository: hbase-git > > > Description > ------- > > This is a step toward final compacting memstore that allowes two modes of > work: index-compaction and data-compaction. > > The index-compaction means that when the new segment is pushed into the > pipeline, it is flattened and probably merged with old segments in the > pipeline. The new merge "feature" induces no data-copy-compaction and no > speculative SQM scan. > The compacting memstore of the data-compaction type means the usage of the > data-copy-compaction. > > > Diffs > ----- > > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactingMemStore.java > 177f222 > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/CompactionPipeline.java > 6a13f43 > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java > 3ca4b0c > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/ImmutableSegment.java > 12b7916 > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactor.java > 714ffe3 > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorIterator.java > 2eafb42 > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java > PRE-CREATION > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java > PRE-CREATION > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java > PRE-CREATION > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/SegmentFactory.java > 510ebbd > > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/VersionedSegmentsList.java > 2e8bead > > hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java > 211a6d8 > > hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellArrayMapMemStore.java > fefe2c1 > > hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestPerColumnFamilyFlush.java > 6bfaa59 > > hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestWalAndCompactingMemStoreFlush.java > 74826b0 > > Diff: https://reviews.apache.org/r/51785/diff/ > > > Testing > ------- > > > Thanks, > > Anastasia Braginsky > >
