HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot #3851
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Nov 17, 2021
| public Future<Void> cloneSnapshotAsync(String snapshotName, TableName tableName, | ||
| boolean restoreAcl) throws IOException, TableExistsException, RestoreSnapshotException { | ||
| return admin.cloneSnapshot(snapshotName, tableName, restoreAcl); | ||
| @Override public Future<Void> cloneSnapshotAsync(String snapshotName, TableName tableName, |
There was a problem hiding this comment.
Nits: @Override in a separated line
| * @param snapshotName name of the snapshot to be cloned | ||
| * @param tableName name of the table where the snapshot will be restored | ||
| * @param restoreAcl <code>true</code> to clone acl into newly created table | ||
| * @param cloneSFT specify the StroreFileTracker implementation used for the table |
There was a problem hiding this comment.
The parameter name is a bit confusing to me. It seems to mean whether to clone the SFT as clone is a verb...
Maybe just call it storeFileTrackerImpl?
There was a problem hiding this comment.
I would prefer not to use storeFileTrackerImpl since we use a Trackers enum value and not the Impl class name. What do you think about customSFT?
There was a problem hiding this comment.
But the javadoc says "specify the StroreFileTracker implementation"? Anyway, customSFT is better than cloneSFT, +1 on changing to customSFT.
| boolean restoreAcl) { | ||
| return wrap(rawAdmin.restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl)); | ||
| boolean restoreAcl) { | ||
| return wrap( |
There was a problem hiding this comment.
No, reverting it.
| @@ -71,6 +75,7 @@ | |||
| private TableDescriptor tableDescriptor; | |||
| private SnapshotDescription snapshot; | |||
| private boolean restoreAcl; | |||
| private String cloneSFT; | |||
There was a problem hiding this comment.
I think we need to persist this in the procedure state data?
There was a problem hiding this comment.
Thanks for pointing it out! I'm adding this.
There was a problem hiding this comment.
Looking at the code, I think there is already a bug in CloneSnapshotProcedure. We do not persist restoreAcl...
Let me file another issue to fix it.
wchevreuil
reviewed
Nov 17, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
Outdated
Show resolved
Hide resolved
| @@ -877,9 +943,14 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final | |||
| */ | |||
| private long restoreSnapshot(final SnapshotDescription reqSnapshot, final TableName tableName, | |||
| final SnapshotDescription snapshot, final TableDescriptor snapshotTableDesc, | |||
| final NonceKey nonceKey, final boolean restoreAcl) throws IOException { | |||
| final NonceKey nonceKey, final boolean restoreAcl) | |||
| throws IOException { | |||
There was a problem hiding this comment.
nit: unnecessary line break
wchevreuil
reviewed
Nov 17, 2021
Apache9
reviewed
Nov 19, 2021
| protected void run() throws IOException { | ||
| setProcId( | ||
| getSnapshotManager().restoreOrCloneSnapshot(snapshotDesc, getNonceKey(), restoreAcl)); | ||
| @Override protected void run() throws IOException { |
There was a problem hiding this comment.
Better update the formatter config of your IDE? The annotation should be on a seprated line...
There was a problem hiding this comment.
Yes, it's rather annoying
| // check if a config change would change the behavior | ||
| static String checkIncompatibleConfig(String currentValue, String newValue, String defaultValue, | ||
| String errorMessage) throws RestoreSnapshotException { | ||
| Boolean hasIncompatibility = false; |
There was a problem hiding this comment.
Why Boolean instead of boolean?
| Boolean hasIncompatibility = false; | ||
| //if there is no current override and the snapshot has an override that does not match the | ||
| //default | ||
| if (StringUtils.isEmpty(currentValue) && !StringUtils.isEmpty(newValue) && |
There was a problem hiding this comment.
I prefer we write the code like this:
if (a != null) {
if (b != null) {
blabla
} else {
blabla
}
} else {
if (b != null) {
blabla
} else {
blabla
}
}
The current logic is a bit hard to understand to me...
There was a problem hiding this comment.
Generally I would agree and I started to write it like that, breaking it down to single checks, but it got huge and from all the numerous outcomes only 3 was interesting to us. Consider the first part:
if (StringUtils.isEmpty(currentValue)) {
if (!StringUtils.isEmpty(newValue)) {
if (!defaultValue.equals(newValue)){
hasIncompatibility = true;
}
else {
//do nothing
}
}
else {
//do nothing
}
}
I never use the else cases and only care if all 3 conditions are true. At that point it felt cleaner to just make it a single check with all 3 conditions.
Alternately I could do something like:
if (StringUtils.isEmpty(currentValue)) {
if (!StringUtils.isEmpty(newValue) && !defaultValue.equals(newValue)) {
hasIncompatibility = true;
}
}
//if there is a current override
else if (!StringUtils.isEmpty(currentValue)) {
// we can only remove the override if it matches the default
if (StringUtils.isEmpty(newValue)) {
if (!defaultValue.equals(currentValue)){
hasIncompatibility = true;
}
}
// the new value have to match the current one
else if (!StringUtils.isEmpty(newValue)) {
if (!currentValue.equals(newValue)) {
hasIncompatibility = true;
}
}
}
But even here we have if statements that have no else cause so they could be just merged back to the check above them like I originally did.
What do you think?
There was a problem hiding this comment.
OK, reviewed the code again, I think the difficulty here is that, we are not comparing only two values, there is a default value too. Then I suggest you use the trick in the checking method in StoreFileTrackerFactory, such as
Where we merge the base configuration with table descriptor and cf descriptor first, and then get the store file tracker implementation from the merged configuration object. In this way we do not need to take care of the override logic as the mergeConfigurations method does it for us.
Of course it will be slower as we need to merge all the configuration values, not only for store file tracker, but I do not think speed of this check is the bottleneck of a restoreSnapshot operation.
There was a problem hiding this comment.
You are right, that simplifies the logic.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Any updates here? Thanks. |
…racking logic Signed-off-by: Wellington Chevreuil <[email protected]>
Signed-off-by: Wellington Chevreuil <[email protected]>
) Signed-off-by: Duo Zhang <[email protected]>
…g from different store file tracker implementations (apache#3656) Signed-off-by: Wellington Chevreuil <[email protected]>
…iptor when creating table (apache#3666) Signed-off-by: Duo Zhang <[email protected]>
… file tracker implementation (apache#3665) Signed-off-by: Wellington Chevreuil <[email protected]>
… tracker (apache#3681) Signed-off-by: Josh Elser <[email protected]>
Signed-off-by: Wellington Chevreuil <[email protected]> Reviewed-by: Josh Elser <[email protected]>
apache#3721) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
…et method (apache#3774) Signed-off-by: Wellington Chevreuil <[email protected]>
… impl (apache#3749) Signed-off-by: Duo Zhang <[email protected]>
…ations to TableDescriptor for existing tables (apache#3700) Signed-off-by: Duo Zhang <[email protected]> Reviewed-by: Wellington Ramos Chevreuil <[email protected]>
…he#3786) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]> Signed-off-by: Wellington Ramos Chevreuil <[email protected]>
apache#3845) Signed-off-by: Duo Zhang <[email protected]>
|
I've rebased HBASE-26067 to the newest master. For addressing the UT problem so we can run most of UTs in the nightly job. Please rebase the PR. Thanks. |
…racking logic Signed-off-by: Wellington Chevreuil <[email protected]>
) Signed-off-by: Duo Zhang <[email protected]>
…ring or cloning snapshot add a clone snapshot parameter to force a specific SFT for the new table add a check to restore snapshot that prevents an SFT change that woud break the current setup tests Change-Id: I40702103af386335ea1e98cd2f1ecc27485fbefb
…ring or cloning snapshot rename variable to customSFT add variable to clone procedure serialization Change-Id: Id940fb381cb4b405549d9e6babda300428341294
…ring or cloning snapshot small improvements
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
joshelser
approved these changes
Dec 14, 2021
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
Outdated
Show resolved
Hide resolved
...main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
Show resolved
Hide resolved
| * @param baseConf Current global configuration | ||
| * @throws RestoreSnapshotException if restore would break the current SFT setup | ||
| */ | ||
| public static void checkForRestoreSnapshot(TableDescriptor currentTableDesc, |
There was a problem hiding this comment.
nit: maybe validatePreRestoreSnapshot(..) to better indicate what we're doing (validating the state) and when we are doing it (before a restore_snapshot).
"Validate" is a bit more specific than "check" to me, but I recognize this is also subjective.
Apache9
reviewed
Dec 15, 2021
| * @throws RestoreSnapshotException if snapshot failed to be cloned | ||
| * @throws IllegalArgumentException if the specified table has not a valid name | ||
| */ | ||
| @InterfaceStability.Evolving |
There was a problem hiding this comment.
Let's remove this one. It should be stable, what is the real concern here? The type of customSFT?
...main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
Show resolved
Hide resolved
| sftConfig.set(StoreFileTrackerFactory.TRACKER_IMPL, customSFT); | ||
| StoreFileTrackerFactory.getTrackerClass(sftConfig); | ||
| } | ||
| catch (RuntimeException e){ |
There was a problem hiding this comment.
Catching RuntimeException is a bit ugly... What happens if we just throw the exception out?
On the method layout we could do it later, since the related classes are all IA.Private. There are only two things, one is to remove the IS annotation, we should not use it for IA.Public interface. And another concern is catching a RuntimeException. I do not have big concerns here. I think we could get this in this week and then merge the feature branch back. |
…edure/CloneSnapshotProcedure.java Co-authored-by: Josh Elser <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
…ring or cloning snapshot remove IS.Evolving change customSFt validation error handling rename snapshot restore check
|
@Apache9 I removed the IS annotation, removed the catch RuntimeException, updated the test to match this change and renamed the restore SFT validation method as Josh requested. |
Apache9
approved these changes
Dec 15, 2021
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
the github check appears to have failed due to some failures in org.apache.hadoop.hbase.security.access.TestNamespaceCommands which appear broken in trace/Netty code. I don't think they're related to Szabolcs' change. |
asfgit
pushed a commit
that referenced
this pull request
Dec 16, 2021
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
Apache9
pushed a commit
that referenced
this pull request
Dec 17, 2021
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Dec 22, 2021
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
Apache9
pushed a commit
that referenced
this pull request
Jan 1, 2022
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
Apache9
pushed a commit
that referenced
this pull request
Jan 2, 2022
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
Apache9
pushed a commit
that referenced
this pull request
Jan 2, 2022
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
Apache9
pushed a commit
that referenced
this pull request
Jan 4, 2022
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
apurtell
pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 18, 2022
…ring or cloning snapshot Closes apache#3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
apurtell
pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 19, 2022
…ring or cloning snapshot Closes apache#3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
apurtell
pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 25, 2022
…ring or cloning snapshot Closes apache#3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
apurtell
pushed a commit
to apurtell/hbase
that referenced
this pull request
Mar 26, 2022
…ring or cloning snapshot Closes apache#3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
apurtell
pushed a commit
that referenced
this pull request
Mar 26, 2022
…ring or cloning snapshot Closes #3851 Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Josh Elser <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
add a clone snapshot parameter to force a specific SFT for the new table
add a check to restore snapshot that prevents an SFT change that would
break the current setup
tests