[
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 file.
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 state
6586 // There should be only one file in the ACTIVE state and that is
the new file
i.e. DISCARDED and ACTIVE?
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, 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)
