-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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-28671 Add close method to REST client #5998
Conversation
also expose HttpClientConnectionManager
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -178,6 +179,10 @@ private void initialize(Cluster cluster, Configuration conf, boolean sslEnabled, | |||
extraHeaders.put(HttpHeaders.AUTHORIZATION, "Bearer " + bearerToken.get()); | |||
} | |||
|
|||
if (connManager.isPresent()) { |
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.
connManager.ifPresent(httpClientBuilder::setConnectionManager)
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.
Done.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
} | ||
|
||
@Override | ||
public void finalize() { |
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.
Seems it is not suggested to use finalize in modern java application. Is this necessary?
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.
Realistically, when this object gets GCd, the underlying apache httpClient / pool object will also get GCd, and those should take care of closing themselves properly in its their finalizer, so it is not strictly necessary.
However, I think that closing resources like tcp connections is a valid use case for finalizers, and it's better to clean up explicitly than to rely on the lower layers' finalizers.
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 have done some googling, almost all the related articles highly suggest to not use this feature any more, and the finalize method has been deprecated in java 9.
For me, if there are no strong reasons, we'd better not use this method any more.
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.
OK, Done.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
also expose HttpClientConnectionManager Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 046e9d5)
also expose HttpClientConnectionManager Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 046e9d5)
also expose HttpClientConnectionManager Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 046e9d5)
also expose HttpClientConnectionManager Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 046e9d5)
also expose HttpClientConnectionManager