[
https://issues.apache.org/jira/browse/HBASE-13082?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15033578#comment-15033578
]
Anoop Sam John commented on HBASE-13082:
----------------------------------------
sortCompactedfiles This is already been called on new temp ArrayList. Do we
still need create a new list? May be to be best way is create ImmutableList
here at end for setting to instance variable
private ChoreService choreService;
We dont need to keep a ref to this.
bq.LOG.trace("No compacted files to compact");
Say no compacted files to archive.
{code}
this.storeEngine.getStoreFileManager().addCompactionResults(compactedFiles,
result);
// Mark the files as compactedAway once the storefiles and compactedfiles
list is finalised
// Let a background thread close the actual reader on these compacted
files and also
// ensure to evict the blocks from block cache so that they are no longer
in
// cache
this.storeEngine.getStoreFileManager().markCompactedAway(compactedFiles);
{code}
Do we need 2 API calls for this? Can the marking be done within
addCompactionResults? May be we need a better name for that API then.
{code}
/**
* @return true if the file is compacted and if there are no references
*/
public boolean isReferencedInReads() {
return !(this.compactedAway && refCount.get() == 0);
}
{code}
That looks bad this API checks the status of compactedAway also. We incr
refCount with out any check. Either name the API accordingly or just it return
state of refCount.
{code}
public boolean isCompactedAway() {
if (this.reader != null) {
return this.reader.isCompactedAway();
}
return true;
}
{code}
We will have not null reader on every file every time right? As per this
logic when there is no reader on a file we treat it as compacted away. Return
false instead of true? Seems we use this only in tests. Why because the other
API isReferencedInReads is checking both ref count and compactedAway state!
{code}
StoreFile.Reader r = file.getReader();
if (r != null && !r.isReferencedInReads()) {
{code}
Better add similar API in StoreFile also and use that rather than using
directly from reader.
removeCompactedFiles -> Every time the chore runs, we seems to make a
ThreadPoolExecutor and ExecutorService which seems expensive. Why we need multi
threaded reader close?
checkResetHeap -> This is working with out any lock now. Say flushed was true
and so we did nullifyCurrentHeap(). Another reader by this time created new
heap. Then only the old thread got a chance. This will try set the var to
false. In btw that another flush happened and variable was set to ON. But we
may miss this reset of heap? It will make the ref to that snapshot for longer
time max till all reader on that snapshot is completed. Should be ok as it is
rare case also.. Just saying.
private HRegion region; -> Do we need this ref of HRegion type? Can be Region?
> 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_15.patch, HBASE-13082_16.patch, HBASE-13082_17.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)
