hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "zhangduo (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-10201) Port 'Make flush decisions per column family' to trunk
Date Tue, 28 Oct 2014 11:51:48 GMT

    [ https://issues.apache.org/jira/browse/HBASE-10201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14186735#comment-14186735
] 

zhangduo commented on HBASE-10201:
----------------------------------

{quote}
Oh, seems like this is lower bound. Only familys > than this size get flushed. Thats nice.
Need to rename I'd say. .... DEFAULT_MEMSTORE_COLUMNFAMILY_FLUSH_SIZE_LOWER_BOUND?
{quote}
Done

{quote}
nit: '+ (just as usual). This value should less than half of the total memstore' missing a
'be'
{quote}
Done

{quote}
Are we double-accounting here?
+ // keep track of oldest sequence id of edit in a store.
+ private final ConcurrentMap<Store, AtomicLong> oldestSeqIdOfStore =
+ new ConcurrentHashMap<Store, AtomicLong>();
The oldest seqid to region is being done by the WAL subsystem IIRC. Now we are doing it in
here by Store. Should we do it in the one place only? The WAL is keeping accounting so it
knows when to release WALs that no longer have edits. Does this accounting interfere?
{quote}

I add a comment on it to noticed that there is a duplication. There is also other duplication
in HRegion such as lastFlushSeqId. And for oldestSeqIdOfStore, I think it is better to store
it in FSHLog because it is single-threaded and sequence id is generated in that thread, the
update logic can be more straight forward without any performance issue, and also, only need
to modify one place instead of four places in current solution. But this means we change FSHLog's
tracking unit from Region to Store, there may be a lot of work to do which is not related
to this issue. I think it is better to open another issue to handle the duplication.

{quote}
Add a log WARN here: + if (columnfamilyFlushSize <= 0) { ?
{quote}

This is same with memstoreFlushSize's initialize code above. < 0 is possible if it is not
set in HTableDescriptor.

{quote}
Name 'getMinFlushTimeForAllStores' as 'getEarliestFlushTimeForAllStore'? I think it clearer
that it does if you have 'earlier' in there (as you have it in your comments). In sympathy
add 'latest' into this method name getMaxFlushTimeForAllStores
Do these two above methods need to be public? Can they be package private? Do they need to
be exposed at all? Ditto for this isPerColumnFamilyFlushEnabled and flushcache
{quote}

Renaming is done. getEarliestFlushTimeForAllStore should be public because TestIOFencing use
it(which in another package). For other methods, they can be package private, but I see lots
of other similar methods declared as public...

{quote}
Should be a WARN: + LOG.debug("Disabling selective flushing of Column Families' memstores.");
?
This comment right? Should it be 'region' rather than 'memstore' in some of the below?
+ // We now have to flush the memstore since it has
+ // reached the threshold, however, we might not need
+ // to flush the entire memstore. If there are certain
{quote}
Done

{quote}
Make one log line rather than two:
+ LOG.info("Started memstore flush for " + this + ", current region memstore size "
+ + StringUtils.byteDesc(this.memstoreSize.get()) + ", and " + storesToFlush.size() + "/"
+ + stores.size() + " column families' memstores are being flushed."
+ + ((wal != null) ? "" : "; wal is null, using passed sequenceid=" + myseqid));
+ for (Store store: storesToFlush) {
{quote}
I use a formatter to wrap it automatically. I modify it manually to
{code:title=HRegion.java|borderStyle=solid}
    LOG.info("Started memstore flush for " + this
        + ", current region memstore size " + StringUtils.byteDesc(this.memstoreSize.get())
        + ", and " + storesToFlush.size() + "/"  + stores.size() + " column families' memstores
are being flushed."
        + ((wal != null) ? "" : "; wal is null, using passed sequenceid=" + myseqid));
{code}
Does this meet the requirement?

{quote}
How you justify removing this?
flushSeqId = getNextSequenceId(wal);
{quote}

it is not removed. I move it startCacheFlush.

{quote}
nit: in below....
if ((now - getLastFlushTime() < flushCheckInterval)) {
+ if ((now - getMinFlushTimeForAllStores() < flushCheckInterval)) {
The former is 'LastFlushTime' and the new code is 'MinFlushTime'... which should it be? Do
we intend same thing here?
{quote}
I think it should be getLatestFlushTimeForAllStores, not getEarliestFlushTimeForAllStores.
Although we may not flush all the stores.

{quote}
Instead of + for (AtomicLong oldestSeqId: needToUpdate) {
... can you use what is in AtomicUtils?
{quote}
Use AtomicUtils.updateMin instead. Thanks.

{quote}
THis can't be package private getOldestSeqIdOfStore ?
{quote}
TestPerColumnFamilyFlush need it and is in another package.

{quote}
What is difference between oldest and lowest in below?
Map<byte[], Long> oldestFlushingSeqNumsLocal = null;
Map<byte[], Long> oldestUnflushedSeqNumsLocal = null;
+ Map<byte[], Long> lowestFlushingRegionSequenceIdsLocal = null;
+ Map<byte[], Long> oldestUnflushedRegionSequenceIdsLocal = null;
{quote}
They are just copies of class fields, so just want to keep the same name with the class field
name.
{code}
oldestFlushingSeqNumsLocal = new HashMap<byte[], Long>(this.lowestFlushingRegionSequenceIds);
	
oldestUnflushedSeqNumsLocal = new HashMap<byte[], Long>(this.oldestUnflushedRegionSequenceIds);
{code}
This looks uncomfortable...

{quote}
This patch is looking great. I'd say it should be on by default in master branch at least
and possibly on by default in branch-1 too. Do up nice release note. That it passed all unit
tests is encouraging. Need to run with the it rig to see if dataloss.
{quote}
I change DEFAULT_HREGION_MEMSTORE_PER_COLUMN_FAMILY_FLUSH to true in the new patch, so it
is turn on by default. I will run integration test to see if there is dataloss.

Thanks very much for reviewing the patch so carefully.

> Port 'Make flush decisions per column family' to trunk
> ------------------------------------------------------
>
>                 Key: HBASE-10201
>                 URL: https://issues.apache.org/jira/browse/HBASE-10201
>             Project: HBase
>          Issue Type: Improvement
>          Components: wal
>            Reporter: Ted Yu
>            Assignee: zhangduo
>            Priority: Critical
>             Fix For: 2.0.0, 0.99.2
>
>         Attachments: 3149-trunk-v1.txt, HBASE-10201-0.98.patch, HBASE-10201-0.98_1.patch,
HBASE-10201-0.98_2.patch, HBASE-10201.patch, HBASE-10201_1.patch, HBASE-10201_2.patch
>
>
> Currently the flush decision is made using the aggregate size of all column families.
When large and small column families co-exist, this causes many small flushes of the smaller
CF. We need to make per-CF flush decisions.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message