bbeaudreault commented on code in PR #5461:
URL: https://github.com/apache/hbase/pull/5461#discussion_r1356965047
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java:
##########
@@ -718,14 +720,19 @@ public static void
closeRegionSilentlyAndWait(ClusterConnection connection, Serv
public AdminService.BlockingInterface getRsAdmin(final ServerName sn) throws
IOException {
AdminService.BlockingInterface admin = this.rsAdmins.get(sn);
if (admin == null) {
- LOG.debug("New admin connection to " + sn.toString());
- if (sn.equals(master.getServerName()) && master instanceof
HRegionServer) {
- // A master is also a region server now, see HBASE-10569 for details
- admin = ((HRegionServer) master).getRSRpcServices();
- } else {
- admin = this.connection.getAdmin(sn);
- }
- this.rsAdmins.put(sn, admin);
+ return this.rsAdmins.computeIfAbsent(sn, server -> {
+ LOG.debug("New admin connection to " + server.toString());
+ if (server.equals(master.getServerName()) && master instanceof
HRegionServer) {
+ // A master is also a region server now, see HBASE-10569 for details
+ return ((HRegionServer) master).getRSRpcServices();
+ } else {
+ try {
+ return this.connection.getAdmin(server);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
As written, it seems we need _some_ synchronization. A concurrent map is
pointless if you do unsyncrhonized writes on it. You could use
ConcurrentMapUtils.computeIfAbsentEx to get around the exception issue.
That said, looking at the impls of getRSRpcServices and getAdmin, I don't
think we really need to cache these at all. getRSRpcServices directly returns
an instance variable. getAdmin already does it's own caching.
So it might make sense to simply remove the rsAdmins map. @Apache9 what do
you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
