-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28716: Users of QuotaRetriever should pass an existing connection (master) #6065
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-28716: Users of QuotaRetriever should pass an existing connection (master) #6065
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
unit test failure appears unrelated |
ndimiduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| */ | ||
| private boolean isManagedConnection = false; | ||
|
|
||
| QuotaRetriever() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?’
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are non-public methods of an IA.Public class required to go through a deprecation cycle, or only public methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only public methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, then this PR is complying with the deprecation policy now
| */ | ||
| public static QuotaRetriever open(final Configuration conf) throws IOException { | ||
| return open(conf, null); | ||
| public static QuotaRetriever open(final Connection conn) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the deprecation cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pankaj72981
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 LGTM
| */ | ||
| public static QuotaRetriever open(final Configuration conf) throws IOException { | ||
| return open(conf, null); | ||
| public static QuotaRetriever open(final Connection conn) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
| 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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, you got it!
ndimiduk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @charlesconnell !
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Unit test failure looks unrelated |
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]>
#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]>
#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]>
apache#6065) (#107) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Nick Dimiduk <[email protected]>
apache#6065) Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Pankaj Kumar <[email protected]> Co-authored-by: Charles Connell <[email protected]> Signed-off-by: apoonia <[email protected]>
Every call to
HBaseAdmin#getQuota()opens a newConnection, and then closes it. As far as I can tell, this is pointless, since it could use the existingConnectionobject held by theHBaseAdmin. Change this and other uses of QuotaRetriever to use existing Connections.