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 2022/07/28 00:00:55 UTC
[GitHub] [hbase] alexdongli0829 opened a new pull request, #4661: [HBASE-27245]Expose the archive API to the end user
alexdongli0829 opened a new pull request, #4661:
URL: https://github.com/apache/hbase/pull/4661
https://issues.apache.org/jira/browse/HBASE-27245
--
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] alexdongli0829 commented on a diff in pull request #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
alexdongli0829 commented on code in PR #4661:
URL: https://github.com/apache/hbase/pull/4661#discussion_r943319819
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -2431,11 +2431,35 @@ private static boolean isCatalogTable(final TableName tableName) {
return tableName.equals(TableName.META_TABLE_NAME);
}
+ private void checkSnapshot(TableName tableName, boolean archive) throws IOException {
+ /*
+ * If decide to delete the table without archive, need to make sure the table has no snapshot
+ * Meanwhile, the check will scan the snapshots which will do list and open, if there is lots of
+ * snapshot, the performance may be impacted, should evaluate the performance between directly
+ * archive and snapshot scan TODO: find some any way to get if the table snapshotted or not
+ */
+ if (!archive) {
Review Comment:
@Apache9 Thanks so much for your time. The thought is because there will be no archive for all the hfiles, so if there are any snapshots, the snapshots will be broken if any refer for the hfiles, so to avoid the broken, I made the check before conduct the actual delete without archive, make sense?
--
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] alexdongli0829 commented on a diff in pull request #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
alexdongli0829 commented on code in PR #4661:
URL: https://github.com/apache/hbase/pull/4661#discussion_r943319819
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -2431,11 +2431,35 @@ private static boolean isCatalogTable(final TableName tableName) {
return tableName.equals(TableName.META_TABLE_NAME);
}
+ private void checkSnapshot(TableName tableName, boolean archive) throws IOException {
+ /*
+ * If decide to delete the table without archive, need to make sure the table has no snapshot
+ * Meanwhile, the check will scan the snapshots which will do list and open, if there is lots of
+ * snapshot, the performance may be impacted, should evaluate the performance between directly
+ * archive and snapshot scan TODO: find some any way to get if the table snapshotted or not
+ */
+ if (!archive) {
Review Comment:
@Apache9 Thanks so much for your time. The thought is because there will be no archive for all the hfiles, so if there are any snapshots, the snapshots will be broken, so to avoid the broken, I made the check before conduct the actual delete without archive, make sense?
--
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 #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4661:
URL: https://github.com/apache/hbase/pull/4661#issuecomment-1197538581
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 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. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 49s | master passed |
| +1 :green_heart: | compile | 5m 40s | master passed |
| +1 :green_heart: | checkstyle | 1m 40s | master passed |
| +1 :green_heart: | spotless | 0m 47s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 7m 2s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 9s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 34s | the patch passed |
| +1 :green_heart: | compile | 5m 52s | the patch passed |
| +1 :green_heart: | cc | 5m 52s | the patch passed |
| +1 :green_heart: | javac | 5m 52s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 37s | the patch passed |
| -0 :warning: | rubocop | 0m 8s | The patch generated 5 new + 407 unchanged - 0 fixed = 412 total (was 407) |
| -0 :warning: | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | hadoopcheck | 13m 0s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | hbaseprotoc | 2m 20s | the patch passed |
| +1 :green_heart: | spotless | 0m 49s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 7m 56s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 45s | The patch does not generate ASF License warnings. |
| | | 61m 22s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4661 |
| JIRA Issue | HBASE-27245 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile cc hbaseprotoc prototool rubocop |
| uname | Linux 9c42e62a7ed6 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a3eeab8c56 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| rubocop | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/1/artifact/yetus-general-check/output/diff-patch-rubocop.txt |
| whitespace | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/1/artifact/yetus-general-check/output/whitespace-eol.txt |
| Max. process+thread count | 60 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-testing-util hbase-thrift hbase-shell U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/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] alexdongli0829 commented on a diff in pull request #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
alexdongli0829 commented on code in PR #4661:
URL: https://github.com/apache/hbase/pull/4661#discussion_r943323866
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java:
##########
@@ -1575,6 +1575,20 @@ public void deleteTable(TableName tableName) throws IOException {
getAdmin().deleteTable(tableName);
}
+ /**
+ * Drop an existing table
+ * @param tableName existing table
+ * @param archive if archive the data when delete the table
+ */
+ public void deleteTable(TableName tableName, boolean archive) throws IOException {
Review Comment:
@Apache9 Correct! Will update the code
--
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 diff in pull request #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4661:
URL: https://github.com/apache/hbase/pull/4661#discussion_r950692907
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -2431,11 +2431,35 @@ private static boolean isCatalogTable(final TableName tableName) {
return tableName.equals(TableName.META_TABLE_NAME);
}
+ private void checkSnapshot(TableName tableName, boolean archive) throws IOException {
+ /*
+ * If decide to delete the table without archive, need to make sure the table has no snapshot
+ * Meanwhile, the check will scan the snapshots which will do list and open, if there is lots of
+ * snapshot, the performance may be impacted, should evaluate the performance between directly
+ * archive and snapshot scan TODO: find some any way to get if the table snapshotted or not
+ */
+ if (!archive) {
Review Comment:
Ah, this is a problem...
I think the correct way is to still archive the files which are referenced by snapshots, and delete unreferenced files directly? This wil be a bit hard as snapshot can happen at the same time?
--
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 diff in pull request #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
Apache9 commented on code in PR #4661:
URL: https://github.com/apache/hbase/pull/4661#discussion_r943293708
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/HBaseTestingUtil.java:
##########
@@ -1575,6 +1575,20 @@ public void deleteTable(TableName tableName) throws IOException {
getAdmin().deleteTable(tableName);
}
+ /**
+ * Drop an existing table
+ * @param tableName existing table
+ * @param archive if archive the data when delete the table
+ */
+ public void deleteTable(TableName tableName, boolean archive) throws IOException {
Review Comment:
Make the above method call this method so we can save some lines of code?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -2431,11 +2431,35 @@ private static boolean isCatalogTable(final TableName tableName) {
return tableName.equals(TableName.META_TABLE_NAME);
}
+ private void checkSnapshot(TableName tableName, boolean archive) throws IOException {
+ /*
+ * If decide to delete the table without archive, need to make sure the table has no snapshot
+ * Meanwhile, the check will scan the snapshots which will do list and open, if there is lots of
+ * snapshot, the performance may be impacted, should evaluate the performance between directly
+ * archive and snapshot scan TODO: find some any way to get if the table snapshotted or not
+ */
+ if (!archive) {
Review Comment:
Why we need this logic here? Mind exlaining a bit?
--
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 #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4661:
URL: https://github.com/apache/hbase/pull/4661#issuecomment-1197642421
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 18s | 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 _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 8s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 38s | master passed |
| +1 :green_heart: | compile | 2m 44s | master passed |
| +1 :green_heart: | shadedjars | 3m 45s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 51s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 37s | the patch passed |
| +1 :green_heart: | compile | 2m 42s | the patch passed |
| +1 :green_heart: | javac | 2m 42s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 44s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 50s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 0m 39s | hbase-protocol-shaded in the patch passed. |
| +1 :green_heart: | unit | 1m 16s | hbase-client in the patch passed. |
| +1 :green_heart: | unit | 209m 52s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 53s | hbase-testing-util in the patch passed. |
| +1 :green_heart: | unit | 7m 0s | hbase-thrift in the patch passed. |
| +1 :green_heart: | unit | 7m 0s | hbase-shell in the patch passed. |
| | | 253m 44s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4661 |
| JIRA Issue | HBASE-27245 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux dc8cc1455888 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a3eeab8c56 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/1/testReport/ |
| Max. process+thread count | 2594 (vs. ulimit of 30000) |
| modules | C: hbase-protocol-shaded hbase-client hbase-server hbase-testing-util hbase-thrift hbase-shell U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4661/1/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] alexdongli0829 commented on a diff in pull request #4661: [HBASE-27245]Expose the archive API to the end user
Posted by GitBox <gi...@apache.org>.
alexdongli0829 commented on code in PR #4661:
URL: https://github.com/apache/hbase/pull/4661#discussion_r952048929
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java:
##########
@@ -2431,11 +2431,35 @@ private static boolean isCatalogTable(final TableName tableName) {
return tableName.equals(TableName.META_TABLE_NAME);
}
+ private void checkSnapshot(TableName tableName, boolean archive) throws IOException {
+ /*
+ * If decide to delete the table without archive, need to make sure the table has no snapshot
+ * Meanwhile, the check will scan the snapshots which will do list and open, if there is lots of
+ * snapshot, the performance may be impacted, should evaluate the performance between directly
+ * archive and snapshot scan TODO: find some any way to get if the table snapshotted or not
+ */
+ if (!archive) {
Review Comment:
@Apache9 You are correct, there was some compromise for the performance and accuracy. If need to scan all the snapshot files and find out if the hfile referred, there will be another performance impact, meanwhile, for the snapshot which is happening, we may need a get the lock from the snapshot manager to make sure no in processing snapshot referring the file, its another concern.
So current I just simplify the logic and return the message to end user and let user decide if they want to remove the snapshot and then delete the table without archive. Do you think its acceptable? Or we had better check the details with some performance sacrifice?
--
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