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 Mon, 23 Nov 2015 06:23:11 GMT

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

stack commented on HBASE-13082:
-------------------------------

Looking at v14:

Yeah, talk about how stuff is different now in the javadoc on compactedfiles... of how now
we keep the files that were compacted away around rather than immediately archive IFF referenced
by scanners.  Do it up here where compactedfiles is introduced.

ImmutableList is a guava-ism? We want to use this in our APIs? (They tend to change faster
than we do and we are on an old version of Guava -- we should update)

Yeah, are these guava Immutable* in later versions of Guava?

This bit repeated? Worth making a private method?

	    ArrayList<StoreFile> newCompactedfiles = null;
133	    if (compactedfiles != null) {
134	      newCompactedfiles = Lists.newArrayList(compactedfiles);
135	    } else {
136	      newCompactedfiles = Lists.newArrayList();
137	    }

i.e. return compactedFiles == null? Lists.newArrayList(): Lists.newArrayList(compactedfiles);

Yeah, below has a side-effect. Instead return what was sorted and have the caller assign:

	204	  private void sortAndSetCompactedFiles(List<StoreFile> storeFiles) {
205	    // Sorting may not be really needed here for the compacted files?
206	    Collections.sort(storeFiles, StoreFile.Comparators.SEQ_ID);
207	    compactedfiles = ImmutableList.copyOf(storeFiles);
208	  }

Yeah man, say what this does when it is introduced in HRegion:

815	    // Start the CompactedHFileCleaner here

Why every '2' minutes? Any reason? 5 minutes?

Help me understand, so a file goes into the compactedfiles list but it may still have references...
its refcount needs to go to zero.  Only when its refcount is zero can it be removed... so
when you do this:

	    // check if the references are cleared now by seeing if the ref files are in DISCARDED
state

You are looking at an enum? Why not just look at the refcount? Have a method isReferenced?
Or isAliveStill? Or isInScan?

Hmm... So, we need Store Interface to have 	  public Collection<StoreFile> getCompactedfiles()
{ because.. the cleaner is on the Region level? What if the Cleaner was on the Store level?
We'd not need to expose 'compactedfiles' outside of Store? That could be cleaner?

Yeah, these guava'isms in our API ain't the best? Just return a List<StoreFile> and
the fact that it is a guava ImmutableList is not exposed until someone tries to change the
list externally... then they'll have a surprise.... that'd be fine.

This is a mouthful:

isReadyForCloseAfterCompaction();

Is this added in this patch? If so, closeAfterCompaction? or isCloseAfterCompaction?

And shouldEvictOnClose should be evictOnClose?

What is going on here?

{code}
886	              ArrayList<StoreFile> filesToRemove = new ArrayList<StoreFile>();
887	              if (compacted) {
888	                filesToRemove.add(f);
889	                fs.removeStoreFiles(getFamily().getNameAsString(), filesToRemove);
890	                // we already have the lock here.
891	                getStoreEngine().getStoreFileManager().removeCompactedFiles(filesToRemove);
892	                filesToRemove.clear();
893	                return null;
894	              }
895	              res.add(f);
{code}

We make filesToRemove even if we may not use it it? i.e. compacted is false. We create this
array to hold one file only? Then clear it?

Yeah, this don't make sense anymore, at least not where it is now:

	    // 4. Compute new store size


Who calls this closeAndArchiveCompactedFiles ? The chore thread?

We do a copy here and operate on the copy?

2351	      Collection<StoreFile> copyCompactedfiles = Lists.newArrayList(compactedfiles);
2352	      removeCompactedFiles(copyCompactedfiles);

Can the original change in the meantime? 

What is isReadyForCloseAfterCompaction ? Is it not after all references have gone away? It
reads like it is close of the region or Store but sounds like it is close of the referencing
Scanner?

Why a lock here?

	      lock.writeLock().lock();

..down in archiveAndRemoveCompactedFiles? There are no references to the file, right?

You are only going to expose one of the below, right?, in new patch?

476	  /**
477	   * Closes and archives the compacted files under this store
478	   */
479	  void closeAndArchiveCompactedFiles() throws IOException;
480	
481	  /**
482	   * Close and archive the compacted files under this store
483	   * @param compactedStorefiles the list of compacted files
484	   */
485	   void closeAndArchiveCompactedFiles(Collection<StoreFile> compactedStorefiles)
486	     throws IOException;


Yeah, do we need these at all since the StoreFile is being managed at a higher level?

153	
154	  // Indicates whether the current store file is compacted or not
155	  private enum FileStatus {
156	    ACTIVE, DISCARDED;
157	  }


So, StoreFile#isCompacted, does that belong in StoreFile? Same with the refcounting? It really
belongs in StoreFileInfo but you say that is not available here. Where is accounting being
done then? Where higher up? Can it do refcounting and whether a file that is done?

Yeah, compacted and refcount belong in StoreFileInfo and if not there, then wherever the storefiles
are being referenced from.... doing it in StoreFile is not right.. .this is meta info on the
StoreFile instances. StoreFileManager?

Yeah, change this:

  ImmutableCollection<StoreFile> clearCompactedFiles();

Shoud return Collection<StoreFile> and internally you do the Immutable stuff (good practice)

In StoreFileScanner, when would scanner be null?

      if (scanner != null) {

You don't set it null in close?

Hopefully we get rid of this someday

	  protected volatile boolean flushed = false;

Maybe a timer on scans? If goes on longer than a minute have it return and then clean up compacted
files.... New issue.

Saving what I have so far.



> 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,
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
(v6.3.4#6332)

Mime
View raw message