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