[ https://issues.apache.org/jira/browse/HIVE-16926?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16082995#comment-16082995 ]
Siddharth Seth commented on HIVE-16926: --------------------------------------- Functionally, looks good to me. Minor comments. - umbilicalServer.umbilicalProtocol.pendingClients.putIfAbsent -> Would be a little cleaner to add a method for this, similar to unregisterClient. - {code} + for (String key : umbilicalImpl.pendingClients.keySet()) { + LlapTaskUmbilicalExternalClient client = umbilicalImpl.pendingClients.get(key);{code} Replace with an iterator over the entrySet to avoid the get() ? Also, this pattern is repeated in hearbeat and nodeHeartbeat - could likely be a method. If I'm not mistaken, the shared umbilical server will not be shut down ever? Maybe in a follow up - some of the static classes could be split out. > LlapTaskUmbilicalExternalClient should not start new umbilical server for > every fragment request > ------------------------------------------------------------------------------------------------ > > Key: HIVE-16926 > URL: https://issues.apache.org/jira/browse/HIVE-16926 > Project: Hive > Issue Type: Sub-task > Components: llap > Reporter: Jason Dere > Assignee: Jason Dere > Attachments: HIVE-16926.1.patch, HIVE-16926.2.patch, > HIVE-16926.3.patch, HIVE-16926.4.patch > > > Followup task from [~sseth] and [~sershe] after HIVE-16777. > LlapTaskUmbilicalExternalClient currently creates a new umbilical server for > every fragment request, but this is not necessary and the umbilical can be > shared. -- This message was sent by Atlassian JIRA (v6.4.14#64029)