HBASE-24788: Fix the connection leaks on getting hbase admin from unclosed connection #2162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bharathv
requested changes
Jul 28, 2020
There was a problem hiding this comment.
There seem to be a few more instances of this in the tests. Mind fixing them too?
git grep " ConnectionFactory.createConnection(" | grep getAdmin hbase-it/src/test/java/org/apache/hadoop/hbase/mapreduce/IntegrationTestBulkLoad.java: try (Connection conn = ConnectionFactory.createConnection(conf); Admin admin = conn.getAdmin(); hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java: try (Admin admin = ConnectionFactory.createConnection(hConf).getAdmin()) { hbase-server/src/main/java/org/apache/hadoop/hbase/util/ServerRegionReplicaUtil.java: Admin admin = ConnectionFactory.createConnection(conf).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestReplicaWithCluster.java: Admin admin = ConnectionFactory.createConnection(HTU.getConfiguration()).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMasterReplication.java: ConnectionFactory.createConnection(configurations[masterClusterNumber]).getAdmin()) { hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestMultiSlaveReplication.java: Admin admin1 = ConnectionFactory.createConnection(conf1).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestPerTableCFReplication.java: Admin replicationAdmin = ConnectionFactory.createConnection(conf1).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestReplicationBase.java: hbaseAdmin = ConnectionFactory.createConnection(CONF1).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestGlobalReplicationThrottler.java: Admin admin1 = ConnectionFactory.createConnection(conf1).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestRegionReplicaReplicationEndpoint.java: Admin admin = ConnectionFactory.createConnection(HTU.getConfiguration()).getAdmin(); hbase-server/src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestRegionReplicaReplicationEndpoint.java: Admin admin = ConnectionFactory.createConnection(HTU.getConfiguration()).getAdmin(); hbase-shell/src/main/ruby/hbase/replication_admin.rb: @admin = ConnectionFactory.createConnection(configuration).getAdmin
| */ | ||
| @Override | ||
| public RecordWriter<KEY, Mutation> getRecordWriter(TaskAttemptContext context) | ||
| throws IOException, InterruptedException { | ||
| throws IOException { |
There was a problem hiding this comment.
nit: Check-style might flag this for bad indentation (needs nested indent)
|
|
||
| try (Connection connection = ConnectionFactory.createConnection(conf); | ||
| Admin admin = connection.getAdmin()) { | ||
|
|
There was a problem hiding this comment.
nit: remove extra newline
hbase-server/src/main/java/org/apache/hadoop/hbase/util/ServerRegionReplicaUtil.java
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
sandeepvinayak
changed the title
bharathv
approved these changes
Jul 28, 2020
| AsyncAdmin admin = | ||
| ConnectionFactory.createAsyncConnection(HTU.getConfiguration()).get().getAdmin(); | ||
| AsyncConnection conn = ConnectionFactory.createAsyncConnection(HTU.getConfiguration()).get(); | ||
| AsyncAdmin admin = conn.getAdmin(); |
There was a problem hiding this comment.
this should be a try-with-resources too?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
sandeepvinayak
force-pushed
the
HBASE-24788
branch
from
c2b57d8 to
6b445ad
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Changes look good, +1. |
virajjasani
requested changes
Jul 29, 2020
Comment on lines
179
to
182
| } catch (ReplicationPeerNotFoundException e) { | ||
| LOG.warn( | ||
| "Region replica replication peer id=" + REGION_REPLICA_REPLICATION_PEER + " not exist", | ||
| e); |
There was a problem hiding this comment.
If we catch this Exception, don't we want to create replication peer id?
We can model this with:
try(closeables){
try{
peerConfig = admin.getReplicationPeerConfig(REGION_REPLICA_REPLICATION_PEER);
} catch (ReplicationPeerNotFoundException e){
}
if (peerConfig == null) {
.....
}
}
There was a problem hiding this comment.
yeah, I was about to submit the change for this.
added 2 commits
July 29, 2020 02:03
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixing the connection leaks in hbase due to closing hbase admin without closing connection.