You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/11/16 14:48:32 UTC
[GitHub] [hbase] BukrosSzabolcs opened a new pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
BukrosSzabolcs opened a new pull request #3851:
URL: https://github.com/apache/hbase/pull/3851
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-991285574
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 54s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 16s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 56s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 9m 13s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 41s | HBASE-26067 passed |
| -0 :warning: | patch | 12m 41s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 7s | the patch passed |
| +1 :green_heart: | compile | 3m 58s | the patch passed |
| +1 :green_heart: | javac | 3m 58s | the patch passed |
| +1 :green_heart: | shadedjars | 9m 9s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 3s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 44s | hbase-client in the patch passed. |
| -1 :x: | unit | 209m 53s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 7m 29s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 3s | hbase-shell in the patch passed. |
| | | 273m 0s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux f5b8b0c0df01 4.15.0-162-generic #170-Ubuntu SMP Mon Oct 18 11:38:05 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/testReport/ |
| Max. process+thread count | 3397 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] joshelser commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-989347996
Also, I filed https://issues.apache.org/jira/browse/HBASE-26550 to fix some of the UT issues. New bug since the extra balancer API changes were made, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994868413
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 1s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 29s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 46s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 2s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 42s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 8m 39s | HBASE-26067 passed |
| -0 :warning: | patch | 2m 11s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 55s | the patch passed |
| +1 :green_heart: | compile | 7m 25s | the patch passed |
| +1 :green_heart: | cc | 7m 25s | the patch passed |
| +1 :green_heart: | javac | 7m 25s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 11s | The patch passed checkstyle in hbase-protocol-shaded |
| -0 :warning: | checkstyle | 0m 31s | hbase-client: The patch generated 2 new + 115 unchanged - 0 fixed = 117 total (was 115) |
| +1 :green_heart: | checkstyle | 1m 7s | hbase-server: The patch generated 0 new + 129 unchanged - 9 fixed = 129 total (was 138) |
| +1 :green_heart: | checkstyle | 0m 46s | The patch passed checkstyle in hbase-thrift |
| +1 :green_heart: | checkstyle | 0m 12s | The patch passed checkstyle in hbase-shell |
| +1 :green_heart: | rubocop | 0m 14s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 21m 23s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 51s | the patch passed |
| +1 :green_heart: | spotbugs | 13m 24s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 9s | The patch does not generate ASF License warnings. |
| | | 88m 44s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/9/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux a35f2cd424f5 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/9/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/9/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-992779940
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 29s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 1s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 28s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 49s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 2s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 41s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 8m 32s | HBASE-26067 passed |
| -0 :warning: | patch | 2m 8s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 49s | the patch passed |
| +1 :green_heart: | compile | 7m 2s | the patch passed |
| +1 :green_heart: | cc | 7m 2s | the patch passed |
| +1 :green_heart: | javac | 7m 2s | the patch passed |
| -0 :warning: | checkstyle | 0m 32s | hbase-client: The patch generated 1 new + 115 unchanged - 0 fixed = 116 total (was 115) |
| -0 :warning: | checkstyle | 1m 5s | hbase-server: The patch generated 3 new + 129 unchanged - 9 fixed = 132 total (was 138) |
| +1 :green_heart: | rubocop | 0m 13s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 31s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 4s | the patch passed |
| +1 :green_heart: | spotbugs | 9m 25s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 4s | The patch does not generate ASF License warnings. |
| | | 79m 25s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux d8f0c38f65c6 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-992916028
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 26s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 5s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 12s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 12s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 8s | HBASE-26067 passed |
| -0 :warning: | patch | 11m 16s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 54s | the patch passed |
| +1 :green_heart: | compile | 3m 13s | the patch passed |
| +1 :green_heart: | javac | 3m 13s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 15s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 6s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 48s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 22s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 153m 4s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 6m 42s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 35s | hbase-shell in the patch passed. |
| | | 208m 35s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 5f81b8607f02 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/testReport/ |
| Max. process+thread count | 5163 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994412034
> Looks great to me. I think I agree with the method layout you have here already, Szabolcs.
>
> It would be great to land this change this week, but let's give Duo the time to comment back once more :)
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753396530
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review 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 check above them like I originally did.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r761434523
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review comment:
You are right, that simplifies the logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-970540322
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751831020
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,24 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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
Review comment:
But the javadoc says "specify the StroreFileTracker implementation"? Anyway, customSFT is better than cloneSFT, +1 on changing to customSFT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994690430
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 37s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 6m 15s | HBASE-26067 passed |
| +1 :green_heart: | compile | 4m 46s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 11m 17s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 3m 1s | HBASE-26067 passed |
| -0 :warning: | patch | 15m 13s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 48s | the patch passed |
| +1 :green_heart: | compile | 4m 40s | the patch passed |
| +1 :green_heart: | javac | 4m 40s | the patch passed |
| +1 :green_heart: | shadedjars | 11m 3s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 3m 8s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 17s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 36s | hbase-client in the patch passed. |
| -1 :x: | unit | 11m 26s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 6m 59s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 6s | hbase-shell in the patch passed. |
| | | 82m 2s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 46c2a229fc46 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/testReport/ |
| Max. process+thread count | 2426 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r767920480
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/snapshot/RestoreSnapshotHelper.java
##########
@@ -707,7 +706,9 @@ private void cloneRegion(final RegionInfo newRegionInfo, final Path regionDir,
HRegionFileSystem.openRegionFromFileSystem(conf, fs, tableDir, newRegionInfo, false) :
HRegionFileSystem.createRegionOnFileSystem(conf, fs, tableDir, newRegionInfo);
- StoreFileTracker tracker = StoreFileTrackerFactory.create(conf, true,
+ Configuration sftConf = StoreUtils.createStoreConfiguration(conf, tableDesc,
Review comment:
Nice catch.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -92,7 +92,7 @@ static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
return name != null ? name.name() : clazz.getName();
}
- private static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
+ public static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
Review comment:
So if we move the method to this class, then we do not need to change this modifier.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +859,46 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
Review comment:
I think we have several similar methods in StoreFileTrackerFactory class
https://github.com/apache/hbase/blob/4aa3f47aa295d0c4bd6235c6bc63897136fa9278/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L196
https://github.com/apache/hbase/blob/4aa3f47aa295d0c4bd6235c6bc63897136fa9278/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L238
So let's also move this method there? Just call it checkForRestoreSnapshot?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StroreFileTracker used for the table
+ * @throws IOException if a remote or network exception occurs
+ * @throws TableExistsException if table to be created already exists
+ * @throws RestoreSnapshotException if snapshot failed to be cloned
+ * @throws IllegalArgumentException if the specified table has not a valid name
+ */
+ default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+ String customSFT)
Review comment:
For IA.Public interface, all methods in it should be considered IS.Stable. The IS annotation is only useful when the class is IA.LimitedPrivate. For IA.Private, everything is IS.Unstable. That's my understanding.
And I think a String is OK here, we could do some checks at server side when we actually convert it to a SFT instance, and report back to client if there is something wrong.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,26 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
return Flow.HAS_MORE_STATE;
}
+ /**
+ * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+ * and set the specified SFT on the table level
+ */
+ private void updateTableDescriptorWithSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+ builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
Review comment:
I think we should do this check prior to do any real operations? In prepareClone method? In ModifyTableProcedure, we do the check in prepareModify method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r765345147
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StroreFileTracker used for the table
Review comment:
```suggestion
* @param customSFT specify the StoreFileTracker used for the table
```
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StroreFileTracker used for the table
+ * @throws IOException if a remote or network exception occurs
+ * @throws TableExistsException if table to be created already exists
+ * @throws RestoreSnapshotException if snapshot failed to be cloned
+ * @throws IllegalArgumentException if the specified table has not a valid name
+ */
+ default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+ String customSFT)
Review comment:
Admin is public API, so this is a commitment that using a String for SFT is the path forward. Do we want a String vs. an enum? I'm not positive if our rules say that we can mark just this one method as private/unstable until we are sure SFT is ready for all users.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,26 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
return Flow.HAS_MORE_STATE;
}
+ /**
+ * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+ * and set the specified SFT on the table level
+ */
+ private void updateTableDescriptorWithSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+ builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
Review comment:
Do we have any ability to validate that this is a valid `customSFT` prior to modifying the TableDesc? Maybe a call we could make on `StoreFileTrackerFactory`?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StroreFileTracker used for the table
+ * @throws IOException if a remote or network exception occurs
+ * @throws TableExistsException if table to be created already exists
+ * @throws RestoreSnapshotException if snapshot failed to be cloned
+ * @throws IllegalArgumentException if the specified table has not a valid name
+ */
+ default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+ String customSFT)
Review comment:
Ok, Admin is IA.Public which requires it to be IS.Stable. Rereading the hbase book, I don't think there's anything stopping us from claiming this one method is IA.LimitedPrivate/IS.Evolving. Maybe someone else can verify my thinking.
##########
File path: hbase-shell/src/main/ruby/shell/commands/clone_snapshot.rb
##########
@@ -33,13 +33,17 @@ def help
newly created table.
hbase> clone_snapshot 'snapshotName', 'namespace:tableName', {RESTORE_ACL=>true}
+
+StoreFileTracker implementation used after restore can be set by the following command.
+ hbase> clone_snapshot 'snapshotName', 'namespace:tableName', {CLONE_SFT=>FILE}
Review comment:
should be `{CLONE_SFT=>"FILE"}`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-979708717
Any updates here?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r766668231
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,26 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
return Flow.HAS_MORE_STATE;
}
+ /**
+ * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+ * and set the specified SFT on the table level
+ */
+ private void updateTableDescriptorWithSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+ builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
Review comment:
You are absolutely right, thanks for pointing it out. I'll add a check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r766671146
##########
File path: hbase-shell/src/main/ruby/shell/commands/clone_snapshot.rb
##########
@@ -33,13 +33,17 @@ def help
newly created table.
hbase> clone_snapshot 'snapshotName', 'namespace:tableName', {RESTORE_ACL=>true}
+
+StoreFileTracker implementation used after restore can be set by the following command.
+ hbase> clone_snapshot 'snapshotName', 'namespace:tableName', {CLONE_SFT=>FILE}
Review comment:
Based on other examples the sell prefers single quotes, but you are right those are missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r767965248
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +859,46 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
Review comment:
Sure, I'll move it there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751363816
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java
##########
@@ -488,14 +488,15 @@
@Override
public CompletableFuture<Void> restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot,
- boolean restoreAcl) {
- return wrap(rawAdmin.restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl));
+ boolean restoreAcl) {
+ return wrap(
Review comment:
No, reverting it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753205297
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -2628,10 +2628,10 @@ public long restoreSnapshot(final SnapshotDescription snapshotDesc,
return MasterProcedureUtil.submitProcedure(
new MasterProcedureUtil.NonceProcedureRunnable(this, nonceGroup, nonce) {
- @Override
- protected void run() throws IOException {
- setProcId(
- getSnapshotManager().restoreOrCloneSnapshot(snapshotDesc, getNonceKey(), restoreAcl));
+ @Override protected void run() throws IOException {
Review comment:
Better update the formatter config of your IDE? The annotation should be on a seprated line...
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
Review comment:
Why Boolean instead of boolean?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review 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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-991294322
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 1s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 36s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 53s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 10m 28s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 48s | HBASE-26067 passed |
| -0 :warning: | patch | 14m 8s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 21s | the patch passed |
| +1 :green_heart: | compile | 4m 10s | the patch passed |
| +1 :green_heart: | javac | 4m 10s | the patch passed |
| +1 :green_heart: | shadedjars | 10m 12s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 3s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 49s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 35s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 220m 44s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 9m 15s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 11s | hbase-shell in the patch passed. |
| | | 289m 21s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux fe99337c69a9 4.15.0-142-generic #146-Ubuntu SMP Tue Apr 13 01:11:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/testReport/ |
| Max. process+thread count | 3977 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994992818
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 0s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 8s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 15s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 18s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 8s | HBASE-26067 passed |
| -0 :warning: | patch | 11m 23s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 50s | the patch passed |
| +1 :green_heart: | compile | 3m 12s | the patch passed |
| +1 :green_heart: | javac | 3m 12s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 22s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 5s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 46s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 17s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 167m 32s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 7m 47s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 8m 41s | hbase-shell in the patch passed. |
| | | 225m 44s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/9/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 158cde37a3a3 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/9/testReport/ |
| Max. process+thread count | 3460 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/9/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] wchevreuil commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751289439
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -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 {
Review comment:
nit: unnecessary line break
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -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 {
MasterCoprocessorHost cpHost = master.getMasterCoprocessorHost();
+ //have to check first if restoring the snapshot would break current SFT setup
+ checkSFTCompatibility(master.getTableDescriptors().get(tableName), snapshotTableDesc,
Review comment:
Do we really need all these complex combinations? Could we just check if the SFT conf between current descriptor and snapshot descriptor differs, then fail it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751367807
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,24 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-974131336
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 5s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 5s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 47s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 8m 29s | HBASE-26067 passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 49s | the patch passed |
| -1 :x: | compile | 1m 26s | hbase-server in the patch failed. |
| -0 :warning: | cc | 1m 26s | hbase-server in the patch failed. |
| -0 :warning: | javac | 1m 26s | hbase-server in the patch failed. |
| -0 :warning: | checkstyle | 0m 31s | hbase-client: The patch generated 4 new + 115 unchanged - 0 fixed = 119 total (was 115) |
| -0 :warning: | checkstyle | 1m 6s | hbase-server: The patch generated 9 new + 134 unchanged - 2 fixed = 143 total (was 136) |
| +1 :green_heart: | rubocop | 0m 11s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 24s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 0s | the patch passed |
| +1 :green_heart: | spotbugs | 9m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 6s | The patch does not generate ASF License warnings. |
| | | 77m 30s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux cceefe13793c 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 0ad3556da7 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| cc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 95 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-988880707
The unit test failures are unrelated, I was able to run all of them locally with no issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] joshelser commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-995347354
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-980646644
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994776439
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r769314742
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -92,7 +92,7 @@ static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
return name != null ? name.name() : clazz.getName();
}
- private static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
+ public static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
Review comment:
We need to load at least the implementation classes so we need to use some internal methods in StoreFileTrackerFactory. Anyway, not a blocker here, maybe we could introduce a special SFTChecker class, put it in the same package with SFTFactory, so we do not need to make these methods public while do not need to put the check methods in SFTFactory.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-986240525
The error prone plugin reports a serious problem and also I think the failed UTs are related? Please update the PR to address these problems.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r766667351
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StroreFileTracker used for the table
+ * @throws IOException if a remote or network exception occurs
+ * @throws TableExistsException if table to be created already exists
+ * @throws RestoreSnapshotException if snapshot failed to be cloned
+ * @throws IllegalArgumentException if the specified table has not a valid name
+ */
+ default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+ String customSFT)
Review comment:
I would prefer to keep it String. The current logic do handle both class names provided as strings and enums. While the enum simplifies SFT handling I see no reason to limit pluggability at this point.
What I have found matches your description so I'm adding IS.Evolving for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-970427117
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 4m 23s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 7s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 11s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 48s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 8m 31s | HBASE-26067 passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 48s | the patch passed |
| -1 :x: | compile | 1m 25s | hbase-server in the patch failed. |
| -0 :warning: | cc | 1m 25s | hbase-server in the patch failed. |
| -0 :warning: | javac | 1m 25s | hbase-server in the patch failed. |
| -0 :warning: | checkstyle | 0m 31s | hbase-client: The patch generated 4 new + 115 unchanged - 0 fixed = 119 total (was 115) |
| -0 :warning: | checkstyle | 1m 5s | hbase-server: The patch generated 9 new + 134 unchanged - 2 fixed = 143 total (was 136) |
| +1 :green_heart: | rubocop | 0m 13s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 21s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 3s | the patch passed |
| +1 :green_heart: | spotbugs | 9m 18s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 6s | The patch does not generate ASF License warnings. |
| | | 81m 42s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux c7d884353713 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 36b6088a26 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| cc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753632988
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review 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
https://github.com/apache/hbase/blob/0ad3556da7e71b4719ee41de8fb704b45b390d00/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L238
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-988073118
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 29s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 35s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 54s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 7s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 44s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 9m 11s | HBASE-26067 passed |
| -0 :warning: | patch | 2m 11s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 8s | the patch passed |
| +1 :green_heart: | compile | 7m 48s | the patch passed |
| +1 :green_heart: | cc | 7m 48s | the patch passed |
| +1 :green_heart: | javac | 7m 48s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 11s | The patch passed checkstyle in hbase-protocol-shaded |
| +1 :green_heart: | checkstyle | 0m 30s | The patch passed checkstyle in hbase-client |
| +1 :green_heart: | checkstyle | 1m 8s | hbase-server: The patch generated 0 new + 130 unchanged - 9 fixed = 130 total (was 139) |
| +1 :green_heart: | checkstyle | 0m 44s | The patch passed checkstyle in hbase-thrift |
| +1 :green_heart: | checkstyle | 0m 11s | The patch passed checkstyle in hbase-shell |
| +1 :green_heart: | rubocop | 0m 14s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 22m 20s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 23s | the patch passed |
| +1 :green_heart: | spotbugs | 10m 16s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 57s | The patch does not generate ASF License warnings. |
| | | 87m 5s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux 72dd3791d597 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-991166986
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 59s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 29s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 24s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 55s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 44s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 9m 15s | HBASE-26067 passed |
| -0 :warning: | patch | 2m 12s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 57s | the patch passed |
| +1 :green_heart: | compile | 7m 2s | the patch passed |
| +1 :green_heart: | cc | 7m 2s | the patch passed |
| +1 :green_heart: | javac | 7m 2s | the patch passed |
| +1 :green_heart: | checkstyle | 0m 11s | The patch passed checkstyle in hbase-protocol-shaded |
| -0 :warning: | checkstyle | 0m 31s | hbase-client: The patch generated 1 new + 115 unchanged - 0 fixed = 116 total (was 115) |
| +1 :green_heart: | checkstyle | 1m 5s | hbase-server: The patch generated 0 new + 130 unchanged - 9 fixed = 130 total (was 139) |
| +1 :green_heart: | checkstyle | 0m 43s | The patch passed checkstyle in hbase-thrift |
| +1 :green_heart: | checkstyle | 0m 12s | The patch passed checkstyle in hbase-shell |
| +1 :green_heart: | rubocop | 0m 13s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 27s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 3s | the patch passed |
| +1 :green_heart: | spotbugs | 9m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 5s | The patch does not generate ASF License warnings. |
| | | 82m 19s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux eb14cf1deea6 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/6/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r768820840
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,25 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StroreFileTracker used for the table
+ * @throws IOException if a remote or network exception occurs
+ * @throws TableExistsException if table to be created already exists
+ * @throws RestoreSnapshotException if snapshot failed to be cloned
+ * @throws IllegalArgumentException if the specified table has not a valid name
+ */
+ default void cloneSnapshot(String snapshotName, TableName tableName, boolean restoreAcl,
+ String customSFT)
Review comment:
Ok, if you think a String is OK then I'm OK with that :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994786800
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 28s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 49s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 14s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 15s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 7s | HBASE-26067 passed |
| -0 :warning: | patch | 11m 19s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 54s | the patch passed |
| +1 :green_heart: | compile | 3m 12s | the patch passed |
| +1 :green_heart: | javac | 3m 12s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 19s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 4s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 47s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 20s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 153m 23s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 6m 40s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 28s | hbase-shell in the patch passed. |
| | | 208m 45s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 3c651f513e14 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/testReport/ |
| Max. process+thread count | 5046 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-987994714
Fixed the unit test and the error prone part. Hopefully fixed the conflicts too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r767939110
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,26 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
return Flow.HAS_MORE_STATE;
}
+ /**
+ * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+ * and set the specified SFT on the table level
+ */
+ private void updateTableDescriptorWithSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+ builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
Review comment:
I'm moving it prepareClone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-988210160
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 3s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 32s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 16s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 9m 7s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 59s | HBASE-26067 passed |
| -0 :warning: | patch | 11m 53s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 16s | the patch passed |
| +1 :green_heart: | compile | 3m 9s | the patch passed |
| +1 :green_heart: | javac | 3m 9s | the patch passed |
| +1 :green_heart: | shadedjars | 9m 8s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 58s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 49s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 35s | hbase-client in the patch passed. |
| -1 :x: | unit | 225m 29s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 8m 40s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 8m 22s | hbase-shell in the patch passed. |
| | | 286m 36s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 3dfd5163bbcf 4.15.0-147-generic #151-Ubuntu SMP Fri Jun 18 19:21:19 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/testReport/ |
| Max. process+thread count | 3359 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-988157101
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 40s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 30s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 36s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 46s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 18s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 31s | HBASE-26067 passed |
| -0 :warning: | patch | 11m 47s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 30s | the patch passed |
| +1 :green_heart: | compile | 3m 47s | the patch passed |
| +1 :green_heart: | javac | 3m 47s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 18s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 32s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 2s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 22s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 143m 43s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 6m 54s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 10s | hbase-shell in the patch passed. |
| | | 202m 20s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux c32c0a617f58 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/testReport/ |
| Max. process+thread count | 4397 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/5/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751373627
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -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 {
MasterCoprocessorHost cpHost = master.getMasterCoprocessorHost();
+ //have to check first if restoring the snapshot would break current SFT setup
+ checkSFTCompatibility(master.getTableDescriptors().get(tableName), snapshotTableDesc,
Review comment:
That would not be fully correct. In certain scenarios we would fail even when the restore is possible and would force the user to make config changes that would not have any effect other than satisfying this check's requirements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751374967
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -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 {
Review comment:
Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753396530
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-985117663
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 17s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 42s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 49s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 21s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 36s | HBASE-26067 passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 28s | the patch passed |
| +1 :green_heart: | compile | 3m 48s | the patch passed |
| +1 :green_heart: | javac | 3m 48s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 24s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 31s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 0s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 25s | hbase-client in the patch passed. |
| -1 :x: | unit | 148m 39s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 6m 25s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 21s | hbase-shell in the patch passed. |
| | | 207m 34s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 351d739d8c96 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / d24e09dbb5 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/testReport/ |
| Max. process+thread count | 3877 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751313836
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -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 {
MasterCoprocessorHost cpHost = master.getMasterCoprocessorHost();
+ //have to check first if restoring the snapshot would break current SFT setup
+ checkSFTCompatibility(master.getTableDescriptors().get(tableName), snapshotTableDesc,
Review comment:
I think we need these things. As we can specify SFT on column family...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751831356
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -71,6 +75,7 @@
private TableDescriptor tableDescriptor;
private SnapshotDescription snapshot;
private boolean restoreAcl;
+ private String cloneSFT;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753375599
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##########
@@ -2628,10 +2628,10 @@ public long restoreSnapshot(final SnapshotDescription snapshotDesc,
return MasterProcedureUtil.submitProcedure(
new MasterProcedureUtil.NonceProcedureRunnable(this, nonceGroup, nonce) {
- @Override
- protected void run() throws IOException {
- setProcId(
- getSnapshotManager().restoreOrCloneSnapshot(snapshotDesc, getNonceKey(), restoreAcl));
+ @Override protected void run() throws IOException {
Review comment:
Yes, it's rather annoying
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-985118638
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 29s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 5s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 13s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 18s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 8s | HBASE-26067 passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 19s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 56s | the patch passed |
| +1 :green_heart: | compile | 3m 13s | the patch passed |
| +1 :green_heart: | javac | 3m 13s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 27s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 6s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 47s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 22s | hbase-client in the patch passed. |
| -1 :x: | unit | 153m 47s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 6m 43s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 29s | hbase-shell in the patch passed. |
| | | 209m 40s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux d3c544d403d0 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / d24e09dbb5 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/testReport/ |
| Max. process+thread count | 5337 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r768796706
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,42 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
return Flow.HAS_MORE_STATE;
}
+ /**
+ * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+ * and set the specified SFT on the table level
+ */
+ private void updateTableDescriptorWithSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+ builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
+ for (ColumnFamilyDescriptor family : tableDescriptor.getColumnFamilies()){
+ ColumnFamilyDescriptorBuilder cfBuilder = ColumnFamilyDescriptorBuilder.newBuilder(family);
+ cfBuilder.setConfiguration(StoreFileTrackerFactory.TRACKER_IMPL, null);
+ cfBuilder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, null);
+ builder.modifyColumnFamily(cfBuilder.build());
+ }
+ tableDescriptor = builder.build();
+ }
+
+ private void validateSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ try {
+ Configuration sftConfig = new Configuration();
+ sftConfig.set(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
+ StoreFileTrackerFactory.getTrackerClass(sftConfig);
+ }
+ catch (RuntimeException e){
Review comment:
Catching RuntimeException is a bit ugly... What happens if we just throw the exception out?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -92,7 +92,7 @@ static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
return name != null ? name.name() : clazz.getName();
}
- private static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
+ public static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
Review comment:
Then let's also move the method here?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1634,26 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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 customSFT specify the StoreFileTracker used for the table
+ * @throws IOException if a remote or network exception occurs
+ * @throws TableExistsException if table to be created already exists
+ * @throws RestoreSnapshotException if snapshot failed to be cloned
+ * @throws IllegalArgumentException if the specified table has not a valid name
+ */
+ @InterfaceStability.Evolving
Review comment:
Let's remove this one. It should be stable, what is the real concern here? The type of customSFT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] joshelser closed pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
joshelser closed pull request #3851:
URL: https://github.com/apache/hbase/pull/3851
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-986919929
HBASE-26462 has been merged, I rebased HBASE-26067 because it is conflict with the PR here, as we both need to modify the CloneSnapshotStateData and RestoreSnapshotState. Please update the PR, the new field added here should set the order to 7 instead of 6.
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r751409600
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -71,6 +75,7 @@
private TableDescriptor tableDescriptor;
private SnapshotDescription snapshot;
private boolean restoreAcl;
+ private String cloneSFT;
Review comment:
Thanks for pointing it out! I'm adding this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r750990968
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -1633,7 +1633,24 @@ void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean
*/
default void cloneSnapshot(String snapshotName, TableName tableName)
throws IOException, TableExistsException, RestoreSnapshotException {
- cloneSnapshot(snapshotName, tableName, false);
+ cloneSnapshot(snapshotName, tableName, false, null);
+ }
+
+ /**
+ * Create a new table by cloning the snapshot content.
+ * @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
Review 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?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncHBaseAdmin.java
##########
@@ -488,14 +488,15 @@
@Override
public CompletableFuture<Void> restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot,
- boolean restoreAcl) {
- return wrap(rawAdmin.restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl));
+ boolean restoreAcl) {
+ return wrap(
Review comment:
Do we need this change?
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##########
@@ -644,14 +644,14 @@ public void restoreSnapshot(String snapshotName) throws IOException, RestoreSnap
@Override
public void restoreSnapshot(String snapshotName, boolean takeFailSafeSnapshot, boolean restoreAcl)
- throws IOException, RestoreSnapshotException {
+ throws IOException, RestoreSnapshotException {
get(admin.restoreSnapshot(snapshotName, takeFailSafeSnapshot, restoreAcl));
}
- @Override
- 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,
Review comment:
Nits: `@Override` in a separated line
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -71,6 +75,7 @@
private TableDescriptor tableDescriptor;
private SnapshotDescription snapshot;
private boolean restoreAcl;
+ private String cloneSFT;
Review comment:
I think we need to persist this in the procedure state data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-994692176
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 59s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 30s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 15s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 13s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 53s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 9m 4s | HBASE-26067 passed |
| -0 :warning: | patch | 2m 4s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 12s | the patch passed |
| +1 :green_heart: | compile | 7m 8s | the patch passed |
| +1 :green_heart: | cc | 7m 8s | the patch passed |
| +1 :green_heart: | javac | 7m 8s | the patch passed |
| -0 :warning: | checkstyle | 0m 30s | hbase-client: The patch generated 1 new + 115 unchanged - 0 fixed = 116 total (was 115) |
| -0 :warning: | checkstyle | 1m 12s | hbase-server: The patch generated 3 new + 129 unchanged - 9 fixed = 132 total (was 138) |
| +1 :green_heart: | rubocop | 0m 15s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 21m 29s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 2m 58s | the patch passed |
| +1 :green_heart: | spotbugs | 9m 53s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 53s | The patch does not generate ASF License warnings. |
| | | 84m 18s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux c956880a620d 4.15.0-162-generic #170-Ubuntu SMP Mon Oct 18 11:38:05 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/8/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r767963691
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -92,7 +92,7 @@ static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
return name != null ? name.name() : clazz.getName();
}
- private static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
+ public static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
Review comment:
CustomSFT param validation before snapshot cloning also relies on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-992965375
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 56s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 29s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 58s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 57s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 9m 13s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 35s | HBASE-26067 passed |
| -0 :warning: | patch | 12m 36s | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 2s | the patch passed |
| +1 :green_heart: | compile | 3m 55s | the patch passed |
| +1 :green_heart: | javac | 3m 55s | the patch passed |
| +1 :green_heart: | shadedjars | 9m 5s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 36s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 3s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 43s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 213m 45s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 7m 43s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 12s | hbase-shell in the patch passed. |
| | | 276m 54s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 2abb8338c183 4.15.0-162-generic #170-Ubuntu SMP Mon Oct 18 11:38:05 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 4aa3f47aa2 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/testReport/ |
| Max. process+thread count | 3543 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/7/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r768824485
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/CloneSnapshotProcedure.java
##########
@@ -203,6 +216,42 @@ protected Flow executeFromState(final MasterProcedureEnv env, final CloneSnapsho
return Flow.HAS_MORE_STATE;
}
+ /**
+ * If a StoreFileTracker is specified we strip the TableDescriptor from previous SFT config
+ * and set the specified SFT on the table level
+ */
+ private void updateTableDescriptorWithSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ TableDescriptorBuilder builder = TableDescriptorBuilder.newBuilder(tableDescriptor);
+ builder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
+ for (ColumnFamilyDescriptor family : tableDescriptor.getColumnFamilies()){
+ ColumnFamilyDescriptorBuilder cfBuilder = ColumnFamilyDescriptorBuilder.newBuilder(family);
+ cfBuilder.setConfiguration(StoreFileTrackerFactory.TRACKER_IMPL, null);
+ cfBuilder.setValue(StoreFileTrackerFactory.TRACKER_IMPL, null);
+ builder.modifyColumnFamily(cfBuilder.build());
+ }
+ tableDescriptor = builder.build();
+ }
+
+ private void validateSFT() {
+ if (StringUtils.isEmpty(customSFT)){
+ return;
+ }
+
+ try {
+ Configuration sftConfig = new Configuration();
+ sftConfig.set(StoreFileTrackerFactory.TRACKER_IMPL, customSFT);
+ StoreFileTrackerFactory.getTrackerClass(sftConfig);
+ }
+ catch (RuntimeException e){
+ throw new UnsupportedOperationException("Specified SFT: " + customSFT + " was not recognized",
+ e);
Review comment:
```suggestion
} catch (RuntimeException e) {
throw new UnsupportedOperationException("Specified SFT: " + customSFT + " was not recognized",
e);
```
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -311,4 +312,40 @@ public static void checkForModifyTable(Configuration conf, TableDescriptor oldTa
}
}
}
+
+ /**
+ * Makes sure restoring a snapshot does not break the current SFT setup
+ * follows StoreUtils.createStoreConfiguration
+ * @param currentTableDesc Existing Table's TableDescriptor
+ * @param snapshotTableDesc Snapshot's TableDescriptor
+ * @param baseConf Current global configuration
+ * @throws RestoreSnapshotException if restore would break the current SFT setup
+ */
+ public static void checkForRestoreSnapshot(TableDescriptor currentTableDesc,
Review 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.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java
##########
@@ -92,7 +92,7 @@ static String getStoreFileTrackerName(Class<? extends StoreFileTracker> clazz) {
return name != null ? name.name() : clazz.getName();
}
- private static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
+ public static Class<? extends StoreFileTracker> getTrackerClass(Configuration conf) {
Review comment:
Seems OK to have these public here since `checkForRestoreSnapshot` method is also used in `SnapshotManager`. Seems overkill to add an interface to this Factory just for these two ("internal public api") methods, which is what we'd normally do. For a Factory class, I would expect I could get/create things as the caller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-991285574
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] BukrosSzabolcs commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
BukrosSzabolcs commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753396530
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review 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 check above them like I originally did.
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-974241295
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 32s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 48s | HBASE-26067 passed |
| +1 :green_heart: | compile | 3m 9s | HBASE-26067 passed |
| +1 :green_heart: | shadedjars | 8m 20s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 3s | HBASE-26067 passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 47s | the patch passed |
| +1 :green_heart: | compile | 3m 10s | the patch passed |
| +1 :green_heart: | javac | 3m 10s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 33s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 5s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 46s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 20s | hbase-client in the patch passed. |
| -1 :x: | unit | 154m 49s | hbase-server in the patch failed. |
| +1 :green_heart: | unit | 6m 37s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 19s | hbase-shell in the patch passed. |
| | | 210m 21s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 475eed260abb 4.15.0-156-generic #163-Ubuntu SMP Thu Aug 19 23:31:58 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / 0ad3556da7 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/testReport/ |
| Max. process+thread count | 5027 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/2/console |
| versions | git=2.17.1 maven=3.6.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache9 commented on a change in pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#discussion_r753632988
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/snapshot/SnapshotManager.java
##########
@@ -854,15 +860,75 @@ public long restoreOrCloneSnapshot(final SnapshotDescription reqSnapshot, final
// Execute the restore/clone operation
long procId;
if (master.getTableDescriptors().exists(tableName)) {
- procId = restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey,
- restoreAcl);
+ procId =
+ restoreSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
} else {
procId =
- cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl);
+ cloneSnapshot(reqSnapshot, tableName, snapshot, snapshotTableDesc, nonceKey, restoreAcl,
+ customSFT);
}
return procId;
}
+ // Makes sure restoring a snapshot does not break the current SFT setup
+ // follows StoreUtils.createStoreConfiguration
+ static void checkSFTCompatibility(TableDescriptor currentTableDesc,
+ TableDescriptor snapshotTableDesc, Configuration baseConf) throws RestoreSnapshotException {
+ //have to compare TableDescriptor.values first
+ String tableDefault = checkIncompatibleConfig(currentTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapshotTableDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ baseConf.get(StoreFileTrackerFactory.TRACKER_IMPL, StoreFileTrackerFactory.Trackers.DEFAULT.name()),
+ " the Table " + currentTableDesc.getTableName().getNameAsString());
+
+ // have to check existing CFs
+ for (ColumnFamilyDescriptor cfDesc : currentTableDesc.getColumnFamilies()) {
+ ColumnFamilyDescriptor snapCFDesc = snapshotTableDesc.getColumnFamily(cfDesc.getName());
+ // if there is no counterpart in the snapshot it will be just deleted so the config does
+ // not matter
+ if (snapCFDesc != null) {
+ // comparing ColumnFamilyDescriptor.conf next
+ String cfDefault = checkIncompatibleConfig(
+ cfDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getConfigurationValue(StoreFileTrackerFactory.TRACKER_IMPL), tableDefault,
+ " the config for column family " + cfDesc.getNameAsString());
+
+ // then ColumnFamilyDescriptor.values
+ checkIncompatibleConfig(cfDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL),
+ snapCFDesc.getValue(StoreFileTrackerFactory.TRACKER_IMPL), cfDefault,
+ " the metadata of column family " + cfDesc.getNameAsString());
+ }
+ }
+ }
+
+ // 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;
+ //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) &&
Review 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
https://github.com/apache/hbase/blob/0ad3556da7e71b4719ee41de8fb704b45b390d00/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileTrackerFactory.java#L238
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3851: HBASE-26286: Add support for specifying store file tracker when restoring or cloning snapshot
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3851:
URL: https://github.com/apache/hbase/pull/3851#issuecomment-985055431
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 30s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +0 :ok: | prototool | 0m 0s | prototool was not available. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ HBASE-26067 Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 53s | HBASE-26067 passed |
| +1 :green_heart: | compile | 7m 1s | HBASE-26067 passed |
| +1 :green_heart: | checkstyle | 2m 48s | HBASE-26067 passed |
| +1 :green_heart: | spotbugs | 8m 34s | HBASE-26067 passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 48s | the patch passed |
| -1 :x: | compile | 1m 28s | hbase-server in the patch failed. |
| -0 :warning: | cc | 1m 28s | hbase-server in the patch failed. |
| -0 :warning: | javac | 1m 28s | hbase-server in the patch failed. |
| -0 :warning: | checkstyle | 0m 29s | hbase-client: The patch generated 4 new + 115 unchanged - 0 fixed = 119 total (was 115) |
| -0 :warning: | checkstyle | 1m 7s | hbase-server: The patch generated 18 new + 135 unchanged - 4 fixed = 153 total (was 139) |
| +1 :green_heart: | rubocop | 0m 14s | There were no new rubocop issues. |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 19m 36s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 3m 3s | the patch passed |
| +1 :green_heart: | spotbugs | 9m 20s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 6s | The patch does not generate ASF License warnings. |
| | | 77m 47s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3851 |
| JIRA Issue | HBASE-26286 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux ce970aeae32d 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | HBASE-26067 / d24e09dbb5 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| compile | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| cc | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| javac | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-general-check/output/patch-compile-hbase-server.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-client.txt |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-thrift hbase-shell U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3851/3/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 rubocop=0.80.0 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org