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-13082) Coarsen StoreScanner locks to RegionScanner
Date Fri, 20 Nov 2015 06:13:11 GMT

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

stack commented on HBASE-13082:

An order of magnitude improvement * 2. Not bad [~ramkrishna].  Thanks for the jmh patch too.
Let me look at the actual patch....

bq. The NON_COMPACTED indicates that it is an ACTIVE file which can be used in reads. COMPACTED
indicates that it is no longer an active file and its contents are already copied to a new

In this patch you still talk of 'compacted'... 

58	   * List of compacted files inside this store
59	   */
60	  private volatile ImmutableList<StoreFile> compactedfiles = null;

These are files that are not to be included in a Scan, right. Need to update comment and rename
variable else we'll stay confused.

BUT, reading the patch and code I see why you are calling them compacted and think you are
right to do do. You just need to explain better what they are when there is no context around
(e.g. in comment on data member... no need to explain when inside addCompactionResult because
plenty context here).

Only one thread involved here?

	  public ImmutableCollection<StoreFile> clearCompactedFiles() {

Suggest you change this method to return the Collection rather than set the data member internall:
i.e remove the 'set' part from sortAndSetCompactedFiles  Do the set on return.  Methods like
this with 'side effects' can be tough to follow.

nit: You know the size of the array to allocate here: 124	      newCompactedFiles = Lists.newArrayList();

Is the below the new 'terminology'? 

6585	    // check if the references are cleared now by seeing if the ref files are in DISCARDED
6586	    // There should be only one file in the ACTIVE state and that is the new file


I don't follow how we were checking for references when we went to merge but in this patch
it changes to a check for compactions:

	6593	    List<StoreFile> compactedFiles = new ArrayList<StoreFile>();
6594	    for (Store s : dstRegion.getStores()) {
6595	      compactedFiles.clear();
6596	      if (!dstRegion.isCompacted((HStore) s, dstRegion.getRegionFileSystem())) {
6597	        throw new IOException("Merged region " + dstRegion
6598	            + " still has files that are not yet compacted, is compaction canceled?");
6599	      }

nit: change this 

	6630	    if (nonCompactedFiles > 1) {
6631	      return false;
6632	    }
6633	    return true;

to: return nonCompactedFiles <= 1;

Fix formatting here abouts if you are doing a new patch: 	    if (!SystemUtils.IS_OS_WINDOWS)

Got to HStore. Will be back.

> Coarsen StoreScanner locks to RegionScanner
> -------------------------------------------
>                 Key: HBASE-13082
>                 URL: https://issues.apache.org/jira/browse/HBASE-13082
>             Project: HBase
>          Issue Type: Bug
>            Reporter: Lars Hofhansl
>            Assignee: ramkrishna.s.vasudevan
>         Attachments: 13082-test.txt, 13082-v2.txt, 13082-v3.txt, 13082-v4.txt, 13082.txt,
13082.txt, HBASE-13082.pdf, HBASE-13082_1.pdf, HBASE-13082_12.patch, HBASE-13082_13.patch,
HBASE-13082_14.patch, HBASE-13082_1_WIP.patch, HBASE-13082_2.pdf, HBASE-13082_2_WIP.patch,
HBASE-13082_3.patch, HBASE-13082_4.patch, HBASE-13082_9.patch, HBASE-13082_9.patch, HBASE-13082_withoutpatch.jpg,
HBASE-13082_withpatch.jpg, LockVsSynchronized.java, gc.png, gc.png, gc.png, hits.png, next.png,
> Continuing where HBASE-10015 left of.
> We can avoid locking (and memory fencing) inside StoreScanner by deferring to the lock
already held by the RegionScanner.
> In tests this shows quite a scan improvement and reduced CPU (the fences make the cores
wait for memory fetches).
> There are some drawbacks too:
> * All calls to RegionScanner need to be remain synchronized
> * Implementors of coprocessors need to be diligent in following the locking contract.
For example Phoenix does not lock RegionScanner.nextRaw() and required in the documentation
(not picking on Phoenix, this one is my fault as I told them it's OK)
> * possible starving of flushes and compaction with heavy read load. RegionScanner operations
would keep getting the locks and the flushes/compactions would not be able finalize the set
of files.
> I'll have a patch soon.

This message was sent by Atlassian JIRA

View raw message