> On Sept. 27, 2016, 10:03 a.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!
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. - Anastasia ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51785/#review150430 ----------------------------------------------------------- On Sept. 26, 2016, 3:27 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, 3:27 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 > >
