HBASE-28716: Users of QuotaRetriever should pass an existing connection (master) by charlesconnell · Pull Request #6065 · apache/hbase

charlesconnell · 2024-07-09T17:34:54Z

Every call to HBaseAdmin#getQuota() opens a new Connection, and then closes it. As far as I can tell, this is pointless, since it could use the existing Connection object held by the HBaseAdmin. Change this and other uses of QuotaRetriever to use existing Connections.

charlesconnell · 2024-07-15T21:44:14Z

unit test failure appears unrelated

ndimiduk

I think this kind of change is the correct thing to do. In principal, our non-service classes should never manage their own Connection instances and instead rely on a caller to provide one. However, we need to execute this change according to our deprecation policy.

ndimiduk · 2024-07-17T09:10:49Z

- */
- private boolean isManagedConnection = false;
-
 QuotaRetriever() {


What a weird API. This class is decorated as IA.Public but it's only created via these static factory method? Any what's with using the default constructor + an init method -- what happened to RAII ?’

Are non-public methods of an IA.Public class required to go through a deprecation cycle, or only public methods?

Only public methods.

Okay, then this PR is complying with the deprecation policy now

ndimiduk · 2024-07-17T09:12:51Z

 */
- public static QuotaRetriever open(final Configuration conf) throws IOException {
- return open(conf, null);
+ public static QuotaRetriever open(final Connection conn) throws IOException {


Because this is IA.Public, you cannot make these blanket changes in one pass. You'll need to observe a deprecation cycle through a major release in order to make breaking changes to the public API.
We document this in depth over on https://hbase.apache.org/book.html#hbase.versioning
If we're going through the trouble to make breaking changes, let's push RAII and do away with the parameterless constructor + init method.

+1 for the deprecation cycle.

If we're going through the trouble to make breaking changes, let's push RAII and do away with the parameterless constructor + init method.
Since we're opening this worm-can, how about we get rid of these static constructor methods and use constructors like a normal object?

pankaj72981

+1 LGTM

ndimiduk · 2024-07-18T11:21:51Z

 */
- public static QuotaRetriever open(final Configuration conf) throws IOException {
- return open(conf, null);
+ public static QuotaRetriever open(final Connection conn) throws IOException {


If we're going through the trouble to make breaking changes, let's push RAII and do away with the parameterless constructor + init method.
Since we're opening this worm-can, how about we get rid of these static constructor methods and use constructors like a normal object?

ndimiduk · 2024-07-18T11:23:54Z

 try (Admin admin = conn.getAdmin()) {
 // Pull all of the tables that have quotas (direct, or from namespace)
- for (QuotaSettings qs : QuotaRetriever.open(conf, filter)) {
+ for (QuotaSettings qs : QuotaRetriever.open(conn, filter)) {


Would you mind also promoting uses of the QuotaRetriever object up to try-with-resource blocks? In this particular case, it looks like we never close the object.

Yep, you got it!

ndimiduk

Thank you @charlesconnell !

Apache-HBase · 2024-07-18T17:04:58Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 14s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 1s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ master Compile Tests _
+0 🆗	mvndep	0m 10s		Maven dependency ordering for branch
+1 💚	mvninstall	2m 52s		master passed
+1 💚	compile	3m 42s		master passed
+1 💚	checkstyle	0m 55s		master passed
+1 💚	spotbugs	2m 16s		master passed
+1 💚	spotless	0m 43s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 11s		Maven dependency ordering for patch
+1 💚	mvninstall	2m 54s		the patch passed
+1 💚	compile	3m 39s		the patch passed
+1 💚	javac	3m 39s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 55s		the patch passed
+1 💚	spotbugs	2m 32s		the patch passed
+1 💚	hadoopcheck	10m 23s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 43s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 21s		The patch does not generate ASF License warnings.
		39m 15s

Subsystem	Report/Notes
Docker	ClientAPI=1.46 ServerAPI=1.46 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6065/4/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#6065
JIRA Issue	HBASE-28716
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux 7e142dc30dea 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `6caf25e`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	86 (vs. ulimit of 30000)
modules	C: hbase-client hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6065/4/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2024-07-18T21:35:13Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 46s		Docker mode activated.
-0 ⚠️	yetus	0m 3s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ master Compile Tests _
+0 🆗	mvndep	0m 16s		Maven dependency ordering for branch
+1 💚	mvninstall	3m 32s		master passed
+1 💚	compile	1m 29s		master passed
+1 💚	javadoc	0m 55s		master passed
+1 💚	shadedjars	6m 15s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+0 🆗	mvndep	0m 12s		Maven dependency ordering for patch
+1 💚	mvninstall	3m 42s		the patch passed
+1 💚	compile	1m 44s		the patch passed
+1 💚	javac	1m 44s		the patch passed
+1 💚	javadoc	0m 53s		the patch passed
+1 💚	shadedjars	6m 26s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
+1 💚	unit	2m 4s		hbase-client in the patch passed.
-1 ❌	unit	276m 17s	/patch-unit-hbase-server.txt	hbase-server in the patch failed.
		309m 27s

Subsystem	Report/Notes
Docker	ClientAPI=1.46 ServerAPI=1.46 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6065/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#6065
JIRA Issue	HBASE-28716
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux e919e2ed1164 5.4.0-186-generic #206-Ubuntu SMP Fri Apr 26 12:31:10 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	master / `6caf25e`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6065/4/testReport/
Max. process+thread count	4449 (vs. ulimit of 30000)
modules	C: hbase-client hbase-server U: .
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6065/4/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

ndimiduk · 2024-07-19T10:42:17Z

Unit test failure looks unrelated

[ERROR] Errors: [ERROR] org.apache.hadoop.hbase.master.assignment.TestRollbackSCP.testFailAndRollback
[ERROR] Run 1: TestRollbackSCP.testFailAndRollback:180 Â» IllegalArgument scheduler queue not empty: MasterProcedureScheduler[running=true,tableMap=TableQueue[key=test,lockStatus=LockAndQueue[exclusiveLock=false,sharedLockCount=0],size=1,namespaceLockStatus=LockAndQueue[exclusiveLock=false,sharedLockCount=0]],tableWaitingMap={},peerMap=<null>,metaMap=<null>,globalMap=<null>]
[ERROR] Run 2: TestRollbackSCP.testFailAndRollback:177 Waiting timed out after [30,000] msec
[ERROR] Run 3: TestRollbackSCP.testFailAndRollback:177 Waiting timed out after [30,000] msec

apache#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org>

#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org>

#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>

apache#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>

#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>

apache#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com>

apache#6065) (#107) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> Co-authored-by: Nick Dimiduk <ndimiduk@apache.org>

apache#6065) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Pankaj Kumar <pankajkumar@apache.org> Co-authored-by: Charles Connell <cconnell@hubspot.com> Signed-off-by: apoonia <apoonia@salesforce.com>

Use existing Connection in all cases with QuotaRetriever
a247c98