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