> On Oct. 16, 2016, 5:13 p.m., Anoop Sam John wrote:
> > hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HeapMemStoreLAB.java,
> >  line 179
> > <https://reviews.apache.org/r/51785/diff/7-8/?file=1511963#file1511963line179>
> >
> >     Able to get what u r trying to do here.  Seems very complex now.
> >     So can we take a diff path?
> >     We have to merge N segments into one here means we have to merge N 
> > MSLABs referred by these segments.  What we do as per this patch is make a 
> > new HeapMSLAB object and add all pooled chunk from other MSLABs int the Q 
> > in new one.  And other stuff of refer next etc.
> >     
> >     Can we make a new MSLAB impl? This will act as a collection of N 
> > HeapMSLABs. Now we have to impl 4 methods
> >     incScannerCount and decScannerCount based on an openScannerCount in the 
> > new class.
> >     close() method like in HeapMSLAB and has to call close() on each of the 
> > N MSLABs it wrap
> >     This is an ImmutableSegment so no one is going to call copyCellInto(). 
> > Make this impl in this new class throw IllegalStateException or so.  Will 
> > that look good?
> Michael Stack wrote:
>     No copying? That sounds good.
> Anoop Sam John wrote:
>     So this fix and so this comment is applicable to merge op alone..  There 
> is still an open point which Me/Ram are not agreeing around this.. That is 
> whether to do this in mem merge.  This is required to limit the #segments it 
> pipeline and to get bigger sized segment at tail of the pipeline. As we do 
> flush of the tail of the segment.  But our point is let us flush all segments 
> (I mean flush to disk)..  In case of in mem compaction (This action is by def 
> OFF and as per data usage one can enable) it is worth to flush only tail.  In 
> this normal use case let us flush all segments and so there is only flatten 
> of segments comes in and so no bug as this. There is absolutely no gain we 
> get because of delaying of flush of the other segments in the pipeline ( I 
> mean in merge kind of usages)..   So I suggest let us discuss on that before 
> going to work on this. Else it wl b waste of time.
> Anastasia Braginsky wrote:
>     I am referencing in this answer the Anoop's suggestion to create a new 
> MSLAB implementation that act as a collection of N old HeapMSLABs. Please pay 
> attention that old scans (opened on N old HeapMSLABs) are not aware of 
> existing of the new MSLAB. In addition the close() of the new MSLAB doesn't 
> automatically means old MSLABs need to be closed, as there might be still 
> ongoing scans. Thus I do not think this idea works (at least not as it is 
> presented now).
> Anastasia Braginsky wrote:
>     Anoop, I do not understand about what exactly you disagree. Are you 
> saying that merging of the segments is waste of time? Do you prefer to do a 
> merge upon the flush to disk? Aa we have seen on stress benchmarks this 
> behavior delays flushes and blocks writes which is undesirable. Please 
> explain yourself clearer or we can talk about if we have a meeting this 
> Wednesday.
> Anoop Sam John wrote:
>     Reply
>     First abt the COllection of N HeapMSLAB wont work.
>     You say problem with old scans which were working on old HeapMSLABs and 
> they dont know abt the new MSLAB.  There wont be an issue.  We will really 
> close an MSLAB (I mean return chunks to pool) when there is a close call 
> happened on it AND the scanner count is zero.  So in the case u mentioned, as 
> there are old scan, the scanner count on these MSLABs are >0. Remember we are 
> not going to call close() on these.  We just have a new Collection based 
> MSLAB impl which point to N old MSLABs.  So the old scanners still use the 
> old MSLABs and once done they decr the counter.
>     Now on the new one there might be new scan reqs.  So we have scanner 
> count in new COllection based MSLAB impl.  When we are at a point to close 
> this new MSLAB, we will call close on it. It will call close() on all old 
> MSLABs. (If its scanner count is zero or else the close on old MSLABs will be 
> delayed till a decr op makes the count to 0)  Now again within the old MSLAB 
> there will be normal close flow.. Iff its scanner count is 0, it will really 
> get closed.  So in a rare case where the new MSLAB is made and it is at a 
> point of getting closed and still a very old scanner still uses old MSLAB, 
> still no issue.. The one old MSLAB will get really closed only when the long 
> pending scan ends.
>     Second abt disagreement of merge
>     Ya. I feel that there should NOT be an in memory merge op at all.  We 
> need to have the compaction op any way.  Only in case of compaction op mode, 
> the on disk flush will flush only tail of the pipeline.  In other case (def 
> case) there will be in memory flushes (flatten op) and we keep adding 
> segments to pipeline and here when a flush to disk req comes, we will flush 
> all segments not just tail of pipeline..  We try to do this merge only to 
> make the #segments in pipeline less and to have a bigger sized tail for 
> pipeline. No need to do merge at any point of time.. The flush will any way 
> use a scanner (With SQM and all) and let us make that scanner to be on all 
> segmnets.
>     Am I making it clear..  Because of 2nd I suggested let us not fix 
> comments regarding merge.

I agree with the N MSLAB impl that Anoop says here. It should work. As i said 
in the comment in the JIRA the place that you have added the new APIs I was not 
sure if they are really correct but NMSLAB impl should work out because ongoing 
scans on old MSLAB should stil use them and the inc/decr operation should work 
on them seemlessly and that once they are added to the new N MSLAB and any new 
scan on them would have any way incremented the count of the oldMSLAB either 
individually or as a whole so some  how they don't get closed.

- ramkrishna

This is an automatically generated e-mail. To reply, visit:

On Oct. 14, 2016, 7:19 a.m., Anastasia Braginsky wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> -----------------------------------------------------------
> (Updated Oct. 14, 2016, 7:19 a.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
>  378601d 
> 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
>  9798ec2 
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreCompactorSegmentsIterator.java
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreMergerSegmentsIterator.java
> hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStoreSegmentsIterator.java
> 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
>  f89a040 
> hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStoreLAB.java
>  1ea5112 
> 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

Reply via email to