> On Oct. 16, 2016, 10:43 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.
>
> ramkrishna vasudevan wrote:
> 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.
>
> Edward Bortnikov wrote:
> Just a general comment -
>
> It's important to keep the mechanism compatible with the original intent
> of doing index or data compaction while the data is in memory. Therefore, we
> can't just delay the segments merge until the flush to disk. The segments
> pipeline should have freedom to compact upon the its own timing (matter of
> policy).
>
> Any design simplification that preserves this degree of freedom is
> welcome.
>
> Anoop Sam John wrote:
> So there should be a policy which allows not to do any merge at all. And
> in that case flush only tail of pipeline is going to a probelm (I think u all
> agreed with that already).. That is why said in such cases, all segments
> should be flushed to disk. There is no adv at all we are getting in delaying
> flush of other regions in such a case.
And this is the collection based MSLAB new impl I was thinking of.. Just tried
to make a class and JFYI placing here.
public class CollectionBackedMemstoreLAB implements MemStoreLAB {
private final AtomicInteger openScannerCount = new AtomicInteger();
private volatile boolean closed = false;
private final List<MemStoreLAB> mslabs;
public CollectionBackedMemstoreLAB(List<MemStoreLAB> mslabs) {
this.mslabs = mslabs;
}
@Override
public Cell copyCellInto(Cell cell) {
throw new IllegalStateException();
}
@Override
public void close() {
this.closed = true;
if (openScannerCount.get() == 0) {
for (MemStoreLAB mslab : this.mslabs) {
mslab.close();
}
}
}
@Override
public void incScannerCount() {
this.openScannerCount.incrementAndGet();
}
@Override
public void decScannerCount() {
int count = this.openScannerCount.decrementAndGet();
if (this.closed && count == 0) {
for (MemStoreLAB mslab : this.mslabs) {
mslab.close();
}
}
}
}
- Anoop Sam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51785/#review152809
-----------------------------------------------------------
On Oct. 14, 2016, 12:49 p.m., Anastasia Braginsky wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51785/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2016, 12:49 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
> 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
> 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
> 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
>
>
