Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-26055 Master local region "table" should be considered a system… #3447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

z-york
Copy link
Contributor

@z-york z-york commented Jun 30, 2021

… table

The master local region is not a typical Table/Region, however, we should consider it a system table when creating the TableName so that if we call isSystemTable() it returns true.

@z-york
Copy link
Contributor Author

z-york commented Jun 30, 2021

One open question with this one: Should we prevent a user from creating a "master" namespace? Currently createNamespace doesn't fail for system namespaces (because it's assumed that the namespace already exists). In this case, the namespace doesn't exist, but the master namespace prefix is being used for a system table.

@z-york z-york requested review from saintstack and Apache9 June 30, 2021 22:18
@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 7s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ master Compile Tests _
+0 🆗 mvndep 0m 23s Maven dependency ordering for branch
+1 💚 mvninstall 3m 51s master passed
+1 💚 compile 4m 4s master passed
+1 💚 checkstyle 1m 28s master passed
+1 💚 spotbugs 2m 47s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 13s Maven dependency ordering for patch
+1 💚 mvninstall 3m 37s the patch passed
+1 💚 compile 4m 0s the patch passed
+1 💚 javac 4m 0s the patch passed
+1 💚 checkstyle 1m 27s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 hadoopcheck 18m 24s Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0.
+1 💚 spotbugs 3m 9s the patch passed
_ Other Tests _
+1 💚 asflicense 0m 27s The patch does not generate ASF License warnings.
53m 6s
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-3447/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #3447
Optional Tests dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname Linux ea775843c42e 4.15.0-65-generic #74-Ubuntu SMP Tue Sep 17 17:06:04 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 147b030
Default Java AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count 96 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/1/console
versions git=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@taklwu
Copy link
Contributor

taklwu commented Jul 1, 2021

The master namespace logic looks good to me, but I have a minor question, do you see anywhere we have any procedure or cleanup logic delete the master namespace ?

Should we prevent a user from creating a "master" namespace? Currently createNamespace doesn't fail for system namespaces (because it's assumed that the namespace already exists)

I found below that the prepareCreate as part fo the CreateNamespaceProcedure should fail if the namespace already exists, and you're right that it didn't protect the system namespace(s).

so, can you explain in what case the system namespaces in the meta table before it comes online would have the pre-existed namespaces? is it from a previous version ?

/**
* Action before any real action of creating namespace.
* @param env MasterProcedureEnv
*/
private boolean prepareCreate(final MasterProcedureEnv env) throws IOException {
if (getTableNamespaceManager(env).doesNamespaceExist(nsDescriptor.getName())) {
setFailure("master-create-namespace",
new NamespaceExistException("Namespace " + nsDescriptor.getName() + " already exists"));
return false;
}
getTableNamespaceManager(env).validateTableAndRegionCount(nsDescriptor);
checkNamespaceRSGroup(env, nsDescriptor);
return true;
}

@Apache9
Copy link
Contributor

Apache9 commented Jul 1, 2021

Do you guys face any problems?

The master local region is not a typical table, so in general it should not be conflict with any real tables or regions. You could have a user level table called master:local, no problem.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 26s Docker mode activated.
-0 ⚠️ 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 🆗 mvndep 0m 21s Maven dependency ordering for branch
+1 💚 mvninstall 4m 15s master passed
+1 💚 compile 1m 24s master passed
+1 💚 shadedjars 9m 6s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 58s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 13s Maven dependency ordering for patch
+1 💚 mvninstall 3m 57s the patch passed
+1 💚 compile 1m 25s the patch passed
+1 💚 javac 1m 25s the patch passed
+1 💚 shadedjars 9m 0s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 0m 56s the patch passed
_ Other Tests _
+1 💚 unit 2m 2s hbase-common in the patch passed.
+1 💚 unit 184m 37s hbase-server in the patch passed.
220m 43s
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-3447/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile
GITHUB PR #3447
Optional Tests javac javadoc unit shadedjars compile
uname Linux 384ee11222a9 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality dev-support/hbase-personality.sh
git revision master / 147b030
Default Java AdoptOpenJDK-1.8.0_282-b08
Test Results https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/1/testReport/
Max. process+thread count 2223 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/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.

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 1m 5s Docker mode activated.
-0 ⚠️ yetus 0m 2s Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck
_ Prechecks _
_ master Compile Tests _
+0 🆗 mvndep 0m 21s Maven dependency ordering for branch
+1 💚 mvninstall 5m 0s master passed
+1 💚 compile 1m 45s master passed
+1 💚 shadedjars 9m 5s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 1m 6s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 14s Maven dependency ordering for patch
+1 💚 mvninstall 4m 56s the patch passed
+1 💚 compile 1m 43s the patch passed
+1 💚 javac 1m 43s the patch passed
+1 💚 shadedjars 9m 10s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 1m 4s the patch passed
_ Other Tests _
+1 💚 unit 2m 11s hbase-common in the patch passed.
+1 💚 unit 183m 30s hbase-server in the patch passed.
223m 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-3447/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR #3447
Optional Tests javac javadoc unit shadedjars compile
uname Linux 7830fd98ca57 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 master / 147b030
Default Java AdoptOpenJDK-11.0.10+9
Test Results https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/1/testReport/
Max. process+thread count 2429 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/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.

@virajjasani
Copy link
Contributor

virajjasani commented Jul 5, 2021

Do you guys face any problems?

The master local region is not a typical table, so in general it should not be conflict with any real tables or regions. You could have a user level table called master:local, no problem.

I agree with @Apache9. I am kind of bit sceptical about making master local region as system table region. I might be missing something but does this improve any particular usecase?
On the other hand, if this has some significant advantage, perhaps we can make it optional feature?

@z-york
Copy link
Contributor Author

z-york commented Jul 6, 2021

@Apache9 @virajjasani Sorry, I was out for the holiday. I understand that this is a special region and it does not have a corresponding table/namespace created for it. We do have an internal patch/feature that makes use of the system table designation (see HBASE-18477). We block writes/updates for user tables, but not system tables (HBASE-18775). Without this flag being set, writes to the master local region will be blocked with our feature. Originally the system table status was stored in the HRegionInfo, but now only the Table knows if it is a system table or not.

I can add a hack/hardcode a check for the master local region. However, it still seems like this is technically a system region? In that case, why would we not denote it as a system region? It certainly is not in the user's domain, it is an internal HBase region.

I'm fine with not adding this to the reserved NS list/restricting creating the NS, but to me it still seems like it is a bug that this "table" (really region) isn't being considered a system table.

@Apache9
Copy link
Contributor

Apache9 commented Jul 12, 2021

@Apache9 @virajjasani Sorry, I was out for the holiday. I understand that this is a special region and it does not have a corresponding table/namespace created for it. We do have an internal patch/feature that makes use of the system table designation (see HBASE-18477). We block writes/updates for user tables, but not system tables (HBASE-18775). Without this flag being set, writes to the master local region will be blocked with our feature. Originally the system table status was stored in the HRegionInfo, but now only the Table knows if it is a system table or not.

I can add a hack/hardcode a check for the master local region. However, it still seems like this is technically a system region? In that case, why would we not denote it as a system region? It certainly is not in the user's domain, it is an internal HBase region.

I'm fine with not adding this to the reserved NS list/restricting creating the NS, but to me it still seems like it is a bug that this "table" (really region) isn't being considered a system table.

It is not a table actually... Neither a user table nor a system table...

So I suggest we should have a way to bypass the newly added logic, and we set this flag or config when constructing the master local region.

Thanks.

Copy link
Contributor

@wchevreuil wchevreuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any plans for the (very hypothetical) case a user namespace named "master" already exists on a cluster running a previous version then decides to upgrade to the version having this change?

@saintstack
Copy link
Contributor

Any resolution here. We going to add a flag to bypass new logic?

@Apache-HBase
Copy link

🎊 +1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 25s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+1 💚 hbaseanti 0m 0s Patch does not have any anti-patterns.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ master Compile Tests _
+0 🆗 mvndep 0m 16s Maven dependency ordering for branch
+1 💚 mvninstall 4m 7s master passed
+1 💚 compile 4m 12s master passed
+1 💚 checkstyle 1m 29s master passed
+1 💚 spotbugs 2m 50s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 14s Maven dependency ordering for patch
+1 💚 mvninstall 3m 52s the patch passed
+1 💚 compile 4m 7s the patch passed
+1 💚 javac 4m 7s the patch passed
+1 💚 checkstyle 1m 26s the patch passed
+1 💚 whitespace 0m 0s The patch has no whitespace issues.
+1 💚 hadoopcheck 19m 39s Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1.
+1 💚 spotbugs 3m 19s the patch passed
_ Other Tests _
+1 💚 asflicense 0m 28s The patch does not generate ASF License warnings.
55m 4s
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-3447/1/artifact/yetus-general-check/output/Dockerfile
GITHUB PR #3447
Optional Tests dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile
uname Linux d82a8b7b2c88 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 master / f6348d4
Default Java AdoptOpenJDK-1.8.0_282-b08
Max. process+thread count 96 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/1/console
versions git=2.17.1 maven=3.6.3 spotbugs=4.2.2
Powered by Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

@Apache-HBase
Copy link

💔 -1 overall

Vote Subsystem Runtime Comment
+0 🆗 reexec 0m 26s Docker mode activated.
-0 ⚠️ 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 🆗 mvndep 0m 16s Maven dependency ordering for branch
+1 💚 mvninstall 4m 45s master passed
+1 💚 compile 1m 42s master passed
+1 💚 shadedjars 8m 29s branch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 1m 9s master passed
_ Patch Compile Tests _
+0 🆗 mvndep 0m 17s Maven dependency ordering for patch
+1 💚 mvninstall 4m 33s the patch passed
+1 💚 compile 1m 42s the patch passed
+1 💚 javac 1m 42s the patch passed
+1 💚 shadedjars 8m 25s patch has no errors when building our shaded downstream artifacts.
+1 💚 javadoc 1m 8s the patch passed
_ Other Tests _
+1 💚 unit 2m 5s hbase-common in the patch passed.
-1 ❌ unit 140m 6s hbase-server in the patch failed.
177m 27s
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-3447/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile
GITHUB PR #3447
Optional Tests javac javadoc unit shadedjars compile
uname Linux e546837b8e7f 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 master / f6348d4
Default Java AdoptOpenJDK-11.0.10+9
unit https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/1/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-3447/1/testReport/
Max. process+thread count 3715 (vs. ulimit of 30000)
modules C: hbase-common hbase-server U: .
Console output https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3447/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants