You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by "Jim Kellerman (JIRA)" <ji...@apache.org> on 2008/02/24 07:08:14 UTC
[jira] Created: (HBASE-469) Streamline HStore startup and
compactions
Streamline HStore startup and compactions
-----------------------------------------
Key: HBASE-469
URL: https://issues.apache.org/jira/browse/HBASE-469
Project: Hadoop HBase
Issue Type: Improvement
Components: regionserver
Affects Versions: 0.2.0
Reporter: Jim Kellerman
Assignee: Jim Kellerman
Fix For: 0.2.0
Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jim Kellerman updated HBASE-469:
--------------------------------
Attachment: patch.txt
FWIW. Still fails in sequential write test.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12584391#action_12584391 ]
Jim Kellerman commented on HBASE-469:
-------------------------------------
Let me know if you want to re-review the patch, or if I can just commit it. Thanks.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-469) Streamline HStore startup and
compactions
Posted by "stack (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12584224#action_12584224 ]
stack commented on HBASE-469:
-----------------------------
Thanks for the detailed explaination.
Patch has lots of nice cleanup.
Here's a couple of comments:
Why catch the exception? Why not just let out?
{code}
+ if (fs == null) {
+ try {
+ this.fs = FileSystem.get(conf);
+ } catch (IOException e) {
+ LOG.fatal("error getting file system", e);
+ throw e;
+ }
{code}
Regards the below:
{code}
+ protected void deleteLocal(File f) {
+ if (f.exists()) {
+ if (f.isDirectory()) {
+ File[] children = f.listFiles();
+ if (children != null && children.length > 0) {
+ for (int i = 0; i < children.length; i++) {
+ deleteLocal(children[i]);
+ }
+ }
+ }
+ f.delete();
+ }
+ }
{code}
Its called deleteLocalFile -- does it only work against local filesystem? Should this method be in file utils instead? Is there a method that does this up in hadoop? What if test is run on DFS?
Should the System.out.* logging be changed in src/test/org/apache/hadoop/hbase/TestHBaseCluster.java?
Whats happening with TestMemcache? Is there a renaming going on? Its different from TestHMemcache?
Why do the below:
{code}
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
{code}
Doesn't a stacktrace come out anyways?
Log instead? + System.err.println("setup completed."); Change all the System.out* to use log?
Just let the below out?
{code}
+ try {
+ r.flushcache();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
+ }
+ try {
+ r.compactStores();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
+ }
{code}
And here:
{code}
+ try {
+ r.flushcache();
+ r.compactStores();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
+ }
{code}
Should this run in tearDown?
{code}
} finally {
mrCluster.shutdown();
+ if (jobConf != null) {
+ deleteLocal(new File(jobConf.get("hadoop.tmp.dir")));
+ }
{code}
Why is this necessary?
{code}
+ void close() {
+ this.lock.writeLock().lock();
+ try {
+ this.memcache.clear();
+ this.snapshot.clear();
+ } finally {
+ this.lock.writeLock().unlock();
+ }
+ }
{code}
Its just busy work and it will mask real memory retention issues -- e.g. if the host of this MemCache is being held on to, then this clearing will make it that condidtion harder to detect (if the host is not being referenced, the memcache and snapshot have no path to root and GC will clean them up). I see that we used do this elsewhere...and you're just tidying stuff.
Can this be final? + protected Memcache memcache = new Memcache();
Why remove the final in below?
- final FileSystem fs;
- private final HBaseConfiguration conf;
+ FileSystem fs;
+ private HBaseConfiguration conf;
Because not set on construction?
This is a nice addition -> + private volatile long storeSize;
I think one of my patches yesterday might make it so this part doesn't apply:
{code}
@@ -208,29 +220,18 @@
if(LOG.isDebugEnabled()) {
LOG.debug("starting " + storeName +
((reconstructionLog == null || !fs.exists(reconstructionLog)) ?
- " (no reconstruction log)" :
- " with reconstruction log: " + reconstructionLog.toString()));
+ " (no reconstruction log)" :
+ " with reconstruction log: " + reconstructionLog.toString()));
{code}
What you think our policy on data member access should be? Sometimes we make things default access. Other times we add accessors:
{code}
+
+ HColumnDescriptor getFamily() {
+ return this.family;
}
{code}
Should we log debug something here just in case things are not working as we'd hope?
{code}
+ } catch (IOException e) {
+ // If the HSTORE_LOGINFOFILE doesn't contain a number, just ignore it.
+ // That means it was built prior to the previous run of HStore, and so
+ // it cannot contain any updates also contained in the log.
+ }
{code}
Just suggesting...
You want to remove this?
{code}
+// this.basedir = null;
+// this.info = null;
+// this.family = null;
+// this.fs = null;
{code}
Would suggest this log not necessary
{code}
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Not compacting " + this.storeName +
+ " because no store files to compact.");
+ }
{code}
Lets log events and changes in state -- not the fact that they did not happen (there are a few other logs after this one). Would suggest that as part of logging compactions, you add rationale.
Should the below check be done inside the checkSplit method for the sake of better coherence?
{code}
+ if (storeSize > this.desiredMaxFileSize) {
+ return checkSplit();
+ }
{code}
This is intentional (Moving where reportOpen happens?)?
{code}
- reportOpen(regionInfo);
}
+ reportOpen(regionInfo);
{code}
I don't think the HRegion cleanup you've added helps (See above note on how I think it could make memleaks harder to detect).
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-469) Streamline HStore startup and
compactions
Posted by "stack (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12585718#action_12585718 ]
stack commented on HBASE-469:
-----------------------------
In review I suggested that we log events, not that events didn't happen.
You counter:
> I think this is useful at debug level.
Here is an example:
{code}
[junit] 2008-04-04 12:15:01,686 INFO [RegionServer:0.compactor] regionserver.HRegion(852): starting compaction on region -ROOT-,,0
[junit] 2008-04-04 12:15:01,687 DEBUG [RegionServer:0.compactor] regionserver.HStore(726): Not compacting 70236052/info because only one store file and it is not a reference
[junit] 2008-04-04 12:15:01,691 INFO [RegionServer:0.compactor] regionserver.HRegion(862): compaction completed on region -ROOT-,,0. Took 0sec
{code}
Thats 3 lines of logging to say compaction did NOT run.
Here are other examples:
{code}
[junit] 2008-04-04 12:15:12,707 DEBUG [RegionServer:0.compactor] regionserver.HStore(719): Not compacting 1578871709/text because no store files to compact.
[junit] 2008-04-04 12:15:12,707 DEBUG [RegionServer:0.compactor] regionserver.HStore(719): Not compacting 1578871709/contents because no store files to compact.
{code}
When compaction does run, it logs. Absence of logging is sufficient for me to know that compaction did not run (I can't think of a reason why I'd want to know why the compaction did not run)
I'd suggest that the less noise in the logs, the easier to decipher whats going on whether we're logging at INFO or DEBUG level and that we only log events... not their NOT having happened
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jim Kellerman updated HBASE-469:
--------------------------------
Attachment: patch.txt
HMerge, HRegionServer
- changes that reflect changes to HRegion, CompactSplitThread and Flusher methods
ServerManager
- Return zero length array to region server if it is exiting or quiesced and Master is not yet ready to shut down.
QueueEntry
- removed. no longer used.
CompactSplitThread
- make compactionQueue a queue of HRegion.
- Add Set<HRegion> so we can quickly determine if a region is in the queue. BlockingQueue.contains() does a linear scan of the queue.
- Add a lock and interruptPolitely methods so that compactions/splits in progress are not interrupted.
- Don't add a region to the queue if it is already present.
Flusher
- change queue from DelayQueue to BlockingQueue, with HRegion entries instead of QueueEntry.
- Add Set<HRegion> to quickly determine if a region is already in the queue to avoid linear scan of BlockingQueue.contains().
- Only put regions in the queue for optional cache flush if the last time they were flushed is older than now - optionalFlushInterval.
- Only add regions to the queue if it is not already present.
HRegion
- don't request a cache flush if one has already been requested.
- In close, null out references so garbage collector will have an easier time figuring out what can be released.
- Add setLastFlushTime so flusher can set it once it has queued an optional flush.
- Replace largestHStore with getLargestHStoreSize: returns long instead of HStoreSize object.
- Add midKey as parameter to splitRegion.
- Reorder start of splitRegion so it doesn't do any work before validating parameters.
- Remove needsSplit and compactIfNeeded - no longer needed.
- compactStores now returns midKey if split is needed.
- snapshotMemcaches now sets flushRequested to false and sets lastFlushTime to now.
- update does not request a cache flush if one has already been requested.
- Override equals and hashCode so HRegions can be stored in a HashSet.
HStore
- loadHStoreFiles now computes max sequence id and the initial size of the store.
- Add getter for family.
- Close nulls out references to for many objects which helps the garbage collector determine if objects can be gc'd.
- internalCacheFlush updates store size, and logs both size of cache flush and resulting map file size (with debug logging enabled).
- Remove needsCompaction and hasReferences - no longer needed.
- compact() returns midKey if store needs to be split.
- compact() does all checking before actually starting a compaction.
- If store size is greater than desiredMaxFileSize, compact returns the midKey for the store regardless of whether a compaction was actually done.
- Added more synchronization in completeCompaction while iterating over storeFiles.
- completeCompaction computes new store size.
- New method checkSplit replaces method size. Returns midKey if store needs to be split and can be split.
HStoreSize
- removed. No longer needed.
Memcache
- added close method which clears all the maps and should make the job easier for garbage collector.
HBaseTestCase
- only set fs if it has not already been set by a subclass.
- Add protected method deleteLocal to clean up files created in local fs, by M/R jobs.
TestTableIndex, TestTableMapReduce
- call new deleteLocal method
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jim Kellerman updated HBASE-469:
--------------------------------
Status: Patch Available (was: In Progress)
Passes all regression tests. Ran PerformanceEvaluation sequentialWrite with 4 clients in 1000MB heap and randomWrite with 4 clients in 2000MB heap (same as 0.1.0)
Please review.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-469) Streamline HStore startup and
compactions
Posted by "stack (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12584406#action_12584406 ]
stack commented on HBASE-469:
-----------------------------
Go commit it Jim
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Work started: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Work on HBASE-469 started by Jim Kellerman.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-469) Streamline HStore startup and
compactions
Posted by "stack (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12583881#action_12583881 ]
stack commented on HBASE-469:
-----------------------------
Before reviewing, does this new patch do as previous versions did -- IIRC -- removing the memory cap for a region rather moving the memory cap down to the store such that if ten stores, we could use ten times the memory we used to use if all stores are undergoing updates?
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jim Kellerman updated HBASE-469:
--------------------------------
Resolution: Fixed
Status: Resolved (was: Patch Available)
Committed.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jim Kellerman updated HBASE-469:
--------------------------------
Attachment: patch.txt
> Here's a couple of comments:
>
> Why catch the exception? Why not just let out?
{code}
+ if (fs == null) {
+ try {
+ this.fs = FileSystem.get(conf);
+ } catch (IOException e) {
+ LOG.fatal("error getting file system", e);
+ throw e;
+ }
{code}
Beats me. Is what we did before. All I really did was encase the existing code inside the if. I have removed the try/catch
> Regards the below:
{code}
+ protected void deleteLocal(File f) {
+ if (f.exists()) {
+ if (f.isDirectory()) {
+ File[] children = f.listFiles();
+ if (children != null && children.length > 0) {
+ for (int i = 0; i < children.length; i++) {
+ deleteLocal(children[i]);
+ }
+ }
+ }
+ f.delete();
+ }
+ }
{code}
> Its called deleteLocalFile - does it only work against local filesystem?
Yes. It is used to clean up the cruft that MapReduce leaves in the local
file system when it runs a test (because it does not clean up after itself)
> Should this method be in file utils instead?
> Is there a method that does this up in hadoop?
Yes, duh! Removed.
> What if test is run on DFS?
It is only used to clean up the cruft that MapReduce leaves in the local file system.
> Should the System.out.* logging be changed in
> src/test/org/apache/hadoop/hbase/TestHBaseCluster.java?
Yes also in TestHRegion.
> Whats happening with TestMemcache? Is there a renaming going on?
> Its different from TestHMemcache?
It was a new test I wrote while debugging. I folded it into TestHMemcache.
> Why do the below:
{code}
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
{code}
> Doesn't a stacktrace come out anyways?
Yes, removed.
> Log instead? + System.err.println("setup completed.");
> Change all the System.out* to use log?
Yes. Done.
> Just let the below out?
{code}
+ try {
+ r.flushcache();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
+ }
+ try {
+ r.compactStores();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
+ }
{code}
> And here:
{code}
+ try {
+ r.flushcache();
+ r.compactStores();
+ } catch (IOException e) {
+ e.printStackTrace();
+ throw e;
+ }
{code}
Done.
> Should this run in tearDown?
{code}
} finally {
mrCluster.shutdown();
+ if (jobConf != null) {
+ deleteLocal(new File(jobConf.get("hadoop.tmp.dir")));
+ }
{code}
Yes. Done.
> Why is this necessary?
{code}
+ void close() {
+ this.lock.writeLock().lock();
+ try {
+ this.memcache.clear();
+ this.snapshot.clear();
+ } finally {
+ this.lock.writeLock().unlock();
+ }
+ }
{code}
> Its just busy work and it will mask real memory retention issues - e.g.
> if the host of this MemCache is being held on to, then this clearing will
> make it that condidtion harder to detect (if the host is not being
> referenced, the memcache and snapshot have no path to root and GC will
> clean them up). I see that we used do this elsewhere...and you're just
> tidying stuff.
I was trying to give the garbage collector some help, but you're right, it's not necessary.
> Can this be final? + protected Memcache memcache = new Memcache();
Yes. Done.
> Why remove the final in below?
{code}
* final FileSystem fs;
* private final HBaseConfiguration conf;
+ FileSystem fs;
+ private HBaseConfiguration conf;
{code}
> Because not set on construction?
No, close at one point nulled out these pointers to try to help the garbage collector. They are final now.
> This is a nice addition -> + private volatile long storeSize;
Yes. HStore for the most part knows how big it is, so why recompute it every time we want to know if the store wants to be split.
> I think one of my patches yesterday might make it so this part doesn't apply:
{code}
@@ -208,29 +220,18 @@
if(LOG.isDebugEnabled()) {
LOG.debug("starting " + storeName +
((reconstructionLog == null || !fs.exists(reconstructionLog)) ?
- " (no reconstruction log)" :
- " with reconstruction log: " + reconstructionLog.toString()));
+ " (no reconstruction log)" :
+ " with reconstruction log: " + reconstructionLog.toString()));
{code}
y, I resolved the conflict.
> What you think our policy on data member access should be? Sometimes we make
> things default access. Other times we add accessors:
{code}
+
+ HColumnDescriptor getFamily() {
+ return this.family;
}
{code}
I don't have a clear opinion on this. If access is package scoped, usually I don't go through an accessor. But if you'd like to formalize this, that's cool with me too.
> Should we log debug something here just in case things are not working as
> we'd hope? Just suggesting...
{code}
+ } catch (IOException e) {
+ // If the HSTORE_LOGINFOFILE doesn't contain a number, just ignore it.
+ // That means it was built prior to the previous run of HStore, and so
+ // it cannot contain any updates also contained in the log.
+ }
{code}
Done.
> You want to remove this?
{code}
+// this.basedir = null;
+// this.info = null;
+// this.family = null;
+// this.fs = null;
{code}
Done.
> Would suggest this log not necessary
{code}
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Not compacting " + this.storeName +
+ " because no store files to compact.");
+ }
{code}
I think this is useful at debug level. If we ever get our INFO/DEBUG messages right, then this should only show up if we are trying to trace a problem.
> Lets log events and changes in state - not the fact that they did not
> happen (there are a few other logs after this one). Would suggest that as
> part of logging compactions, you add rationale.
Like I said above, it we make INFO level logging provide us with enough information, this, as a debug level log, should disappear.
> Should the below check be done inside the checkSplit method for the sake of
> better coherence?
{code}
+ if (storeSize > this.desiredMaxFileSize) {
+ return checkSplit();
+ }
{code}
Yes. Done.
> This is intentional (Moving where reportOpen happens?)?
{code}
- reportOpen(regionInfo);
}
+ reportOpen(regionInfo);
{code}
Yes. Sometimes the master sends multiple open region requests to the region server, why I don't know. If it does and the region server already has the region open, It can make the master happy by telling it that the region has been opened.
> I don't think the HRegion cleanup you've added helps (See above note on
> how I think it could make memleaks harder to detect).
Agreed. Removed.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-469) Streamline HStore startup and
compactions
Posted by "Jim Kellerman (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-469?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12584201#action_12584201 ]
Jim Kellerman commented on HBASE-469:
-------------------------------------
> stack - 31/Mar/08 02:26 PM
> Before reviewing, does this new patch do as previous versions did - IIRC - removing the memory cap
> for a region rather moving the memory cap down to the store such that if ten stores, we could use ten
> times the memory we used to use if all stores are undergoing updates?
No the flushing policy has not changed. That is it still uses checkResources to block updates when the total memcache size for a region reaches 64MB.
The patch for HBASE-69 is the one you're thinking of.
This patch makes the HStore smarter. For example loadHStoreFiles also gets the sequence number and file size while it is loading the stores so a separate scan is no longer necessary. cache flushing updates the store size. The only time the store size is recomputed is after a compaction: in that case compact knows the size of the file it just created so it only has to look at any new store files that might have been created by a cache flush that occurred while the compaction was running.
compact also computes the mid key if the store needs to be (and can be) split even if it decides no compaction is necessary. Thus the compactor thread knows if the region needs to be split after calling compact and does not have to query the region.
QueueEntry's were removed because neither the flusher nor the compactor use delay queues and at that point, the only payload of the QueueEntry was the region reference. HRegion now implements equals and hashCode so it can be inserted into a blocking queue and a hash set. The hash set is a set of all the regions in the queue and can determine if a region is in the queue much faster than BlockingQueue.contains() which does a linear scan of the queue.
> Streamline HStore startup and compactions
> -----------------------------------------
>
> Key: HBASE-469
> URL: https://issues.apache.org/jira/browse/HBASE-469
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: regionserver
> Affects Versions: 0.2.0
> Reporter: Jim Kellerman
> Assignee: Jim Kellerman
> Fix For: 0.2.0
>
> Attachments: patch.txt, patch.txt
>
>
> Several useful patches that streamline HStore startup and compactions that were a part of the abandoned changes in HBASE-69 should be incorporated.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.