mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anoop Sam John <anoop.hb...@gmail.com>
Subject Re: Review Request 51785: [HBASE-16608] Merge for the segments in the compaction pipeline and simplifying the user interface for in-memory flush
Date Wed, 28 Sep 2016 09:35:32 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message