hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "stack (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-10201) Port 'Make flush decisions per column family' to trunk
Date Mon, 27 Oct 2014 19:38:34 GMT

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

Mime
View raw message