[
https://issues.apache.org/jira/browse/HBASE-10201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14185675#comment-14185675
]
stack commented on HBASE-10201:
-------------------------------
Default is DEFAULT_MEMSTORE_COLUMNFAMILY_FLUSH_SIZE 16MB? Where that come
from? If one column family only then we will flush at this lower size rather
than the usual? Should it default to flush size for region?
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?
nit: '+ (just as usual). This value should less than half of the total
memstore' missing a 'be'
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?
Also, sometimes we flush because we want to clear out memstores that are
holding old edits just so we can cleanup our backlog of WAL files. Do you see
this change intefering with this facility (we'd just not pass the
selectiveFlushRequest in this case? (Oh, I see later that you are aware of this
issue and intentionally set selectiveFlushRequest to false when we roll logs --
good).
Add a log WARN here: + if (columnfamilyFlushSize <= 0) { ?
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
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
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) {
How you justify removing this?
- flushSeqId = getNextSequenceId(wal);
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?
Instead of + for (AtomicLong oldestSeqId: needToUpdate) {
... can you use what is in AtomicUtils?
Thanks for doing this:
- long totalSize = 0l;
+ long totalSize = 0L;
THis can't be package private getOldestSeqIdOfStore ?
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;
Hmm... looking at FSHLog., you seem to be doing good accounting by passing in
the stores to flush AND the stores we are not flushing....
Nice test.
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.
> 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
> 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
>
>
> 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)
