[
https://issues.apache.org/jira/browse/HBASE-5547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13266190#comment-13266190
]
[email protected] commented on HBASE-5547:
------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4633/#review7448
-----------------------------------------------------------
Great work. High-level, do we have to have backup/archiving touch all parts of
the code base? Can we not hide it behind more general Interfaces at least when
we get down into Store and Region classes
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveMonitor.java
<https://reviews.apache.org/r/4633/#comment16389>
Tables or hfiles?
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveMonitor.java
<https://reviews.apache.org/r/4633/#comment16384>
Should this interface be at top level? Its used out of the master and
regionserver packages
The implementations could be stay under backup long as we only use the
Interface?
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTableTracker.java
<https://reviews.apache.org/r/4633/#comment16390>
hfiles or tables?
Looks like this class would select hfiles by selected tables
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTableTracker.java
<https://reviews.apache.org/r/4633/#comment16387>
Tracker in hbase usually means tracking znodes up in zookeeper. This
tracks something else. Could confuse. Use different action? Manager
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTableTracker.java
<https://reviews.apache.org/r/4633/#comment16385>
Good. I like that you are taking a 'Server' here rather than a Master or
Regionserver or even a ServerName.
Does this have to be public? Seems like its utility used by the later
HFileArchiveTracker.
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTableTracker.java
<https://reviews.apache.org/r/4633/#comment16386>
Do we have to have a set? Does this Set change during live of this tracker?
Should javadoc say that previous members of Set are cleared out?
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTableTracker.java
<https://reviews.apache.org/r/4633/#comment16388>
good
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16392>
How does this tracker relate to the containing class? They are different
types of trackers? Could confuse?
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16391>
Why pass it in? Why not just create one and keep it running internally?
The class HFileArchiveTableTracker is a little confusing....
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16395>
Want to mention in class comment that user needs to call start, etc., to
start up the tracking?
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16399>
I think it kinda bad that zkw does this... you'd think each tracker would
look after its own stuff (this is not you.. you are folllowing the established
pattern as you should -- I'm just moaning)
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16400>
Is this what ZKUtil.getNodeName does?
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16401>
nit style. if you did if (!path.startsWith...) return; ... you could save
an indent of the whole method. Next time.
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16403>
These znodes have nothing in them... just the tablename of the znode is
used? Nothing to pb then?
src/main/java/org/apache/hadoop/hbase/backup/ServerHFileTableArchiveTracker.java
<https://reviews.apache.org/r/4633/#comment16405>
This is under each table znode? Why we inherit from
HFileArchiveTableTracker?
src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
<https://reviews.apache.org/r/4633/#comment16407>
Is this doing backup? Or making it so the hfiles are not deleted on this
table?
src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java
<https://reviews.apache.org/r/4633/#comment16409>
ditto... this just throws a switch on the don't delete operation?
Something else does the backup?
src/main/java/org/apache/hadoop/hbase/client/HFileArchiveManager.java
<https://reviews.apache.org/r/4633/#comment16410>
table hfiles?
src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java
<https://reviews.apache.org/r/4633/#comment16411>
Why this class have to have both these classes? Can it not just make do w/
Interface? If not, does Interface need broadening?
src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java
<https://reviews.apache.org/r/4633/#comment16412>
Should this be getHFileManager?
Should it be getTableArchivingManager?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4633/#comment16413>
Can this not just be Interface?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4633/#comment16414>
You think this an hfile archive tracker or is it a table archive tracker?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4633/#comment16415>
Does the master need to know anythign about this? Can this be done in the
catalogjanitor completely?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4633/#comment16416>
Is the testing code in same package? IF so, wouldn't need the public
qualifier
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<https://reviews.apache.org/r/4633/#comment16417>
ditto
... this doesn't exist already?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<https://reviews.apache.org/r/4633/#comment16418>
Interface? Odd that we have a reference to this in here.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<https://reviews.apache.org/r/4633/#comment16419>
Could this be a different, more abstract Interface? HFileArchiver with an
'archive' method on it which return true if we are to archive otherwise, false
if its for delete.
Would like to avoid too much of the backup detail coming down deep in here
into HRegion.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<https://reviews.apache.org/r/4633/#comment16420>
Or, could the whole delete/archive thing be hidden behind an hfile disposer
Interface? Pass it hfiles to dispose of. It will delete or archive?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<https://reviews.apache.org/r/4633/#comment16421>
This archiving code shouldn't be in here? Should be in its own class?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
<https://reviews.apache.org/r/4633/#comment16422>
Yeah, seems odd polluting hregion w/ archiving?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/4633/#comment16423>
Can this be Interface?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/4633/#comment16424>
A factory? Is that too much?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/4633/#comment16425>
You should leave these. These are ok by our style convention.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/4633/#comment16426>
Yeah, these are ok... don't change them.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<https://reviews.apache.org/r/4633/#comment16427>
This for tests too?
src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java
<https://reviews.apache.org/r/4633/#comment16428>
Does this have to come out here?
src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
<https://reviews.apache.org/r/4633/#comment16429>
Has to come down in here? Can't this be done at higher up levels?
src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
<https://reviews.apache.org/r/4633/#comment16430>
Ditto on archiving/delete pollution down in here.
src/main/java/org/apache/hadoop/hbase/util/HFileArchiveCleanup.java
<https://reviews.apache.org/r/4633/#comment16431>
These lines have to be this long?
Does this tool belong in backup package?
src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java
<https://reviews.apache.org/r/4633/#comment16432>
Has to reach over into these packages?
src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
<https://reviews.apache.org/r/4633/#comment16433>
This has to be in here?
- Michael
On 2012-05-01 02:42:34, Jesse Yates wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4633/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-05-01 02:42:34)
bq.
bq.
bq. Review request for hbase, Michael Stack and Lars Hofhansl.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Essentially, whenever an hfile would be deleted, it is instead moved to
the archive directory. In this impl, the archive directory is on a per table
basis, but defaults to '.archive'. Removing hfiles occurs in three places -
compaction, merge and catalog janitor. The former and two latter are distinctly
different code paths, but but did pull out some similarities. The latter two
end up calling the same method, so there should be a reasonable amount of
overlap.
bq.
bq. Implementation wise:
bq. Updated the HMasterInterface to pass the calls onto the zookeeper.
bq. Added a zk listener to handle updates from the master to the RS to
backup.
bq. Added a utility for removing files and finding archive directories
bq. Added tests for the regionserver and catalogjanitor approaches.
bq. Added creation of manager in regionserver.
bq.
bq.
bq. This addresses bug HBASE-5547.
bq. https://issues.apache.org/jira/browse/HBASE-5547
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
bq. src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveMonitor.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTableTracker.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/backup/HFileArchiveTracker.java
PRE-CREATION
bq.
src/main/java/org/apache/hadoop/hbase/backup/ServerHFileTableArchiveTracker.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java ee16e72
bq. src/main/java/org/apache/hadoop/hbase/client/HFileArchiveManager.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd
bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java d47b83a
bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 7858846
bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
61a5988
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/RegionServerServices.java
6884d53
bq. src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
ea12da4
bq. src/main/java/org/apache/hadoop/hbase/regionserver/Store.java bf1618e
bq. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveCleanup.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/util/HFileArchiveUtil.java
PRE-CREATION
bq. src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java
4fc105f
bq. src/main/resources/hbase-default.xml f54b345
bq. src/test/java/org/apache/hadoop/hbase/master/MockRegionServer.java
a59e152
bq. src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java
cedf31e
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionHFileArchiving.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/util/HFileArchiveTestingUtil.java
PRE-CREATION
bq. src/test/java/org/apache/hadoop/hbase/util/MockRegionServerServices.java
7d02759
bq.
src/test/java/org/apache/hadoop/hbase/util/TestHFileArchivingCleanup.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/4633/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Added two tests for the separate cases - archiving via the regionserver
and for the catalog tracker. Former runs in a mini cluster and also touches the
changes to HMasterInterface and zookeeper.
bq.
bq.
bq. Thanks,
bq.
bq. Jesse
bq.
bq.
> Don't delete HFiles when in "backup mode"
> -----------------------------------------
>
> Key: HBASE-5547
> URL: https://issues.apache.org/jira/browse/HBASE-5547
> Project: HBase
> Issue Type: New Feature
> Reporter: Lars Hofhansl
> Assignee: Jesse Yates
> Attachments: java_HBASE-5547_v4.patch, java_HBASE-5547_v5.patch,
> java_HBASE-5547_v6.patch
>
>
> This came up in a discussion I had with Stack.
> It would be nice if HBase could be notified that a backup is in progress (via
> a znode for example) and in that case either:
> 1. rename HFiles to be delete to <file>.bck
> 2. rename the HFiles into a special directory
> 3. rename them to a general trash directory (which would not need to be tied
> to backup mode).
> That way it should be able to get a consistent backup based on HFiles (HDFS
> snapshots or hard links would be better options here, but we do not have
> those).
> #1 makes cleanup a bit harder.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
