ttttttz has posted comments on this change. ( http://gerrit.cloudera.org:8080/20358 )
Change subject: IMPALA-10856: Show client hosts and connections in the web UI ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/20358/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20358/3//COMMIT_MSG@9 PS3, Line 9: forms > nit: "tables" Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/rpc/thrift-server.h@184 PS3, Line 184: & > nit: use pointer type for output parameters Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-http-handler.cc@694 PS3, Line 694: lock_guard<mutex> session_state_map_l(server_->session_state_map_lock_); : lock_guard<mutex> connection_to_sessions_map_l( : server_->connection_to_sessions_map_lock_); : lock_guard<mutex> client_hostname_to_connections_map_l( : server_->client_hostname_to_connections_map_lock_); > I'm not sure whether acquiring them together has the risk of dead locks. Th Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-http-handler.cc@807 PS3, Line 807: client_hostname_to_connections_map_ > This map is only used here. Can we construct it using the ConnectionContext Ack This's a good idea! http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-http-handler.cc@829 PS3, Line 829: ++total_sessions; > A session can be used in multiple connections. This seems to over-count the Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-http-handler.cc@857 PS3, Line 857: } > This method is too long now. Let's extract the 3 parts into 3 methods to ma Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-server.h@192 PS3, Line 192: /// * connection_to_sessions_map_lock_ > Please mention 'client_hostname_to_connections_map_lock_' in this section. Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-server.cc@2619 PS3, Line 2619: .insert(connection_context.connection_id); > Why do we insert it twice? One at the begining and one here. Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-server.cc@3454 PS3, Line 3454: ThriftServer::ConnectionContextList > Instead of constructing a vector and copying it out, let's pass in a pointe Ack http://gerrit.cloudera.org:8080/#/c/20358/3/be/src/service/impala-server.cc@3456 PS3, Line 3456: > nit: change double spaces to single space. Same for the following comments. Ack -- To view, visit http://gerrit.cloudera.org:8080/20358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie89015b00e1b97a1836eeca205b2c80b32300227 Gerrit-Change-Number: 20358 Gerrit-PatchSet: 3 Gerrit-Owner: ttttttz <2433038...@qq.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: ttttttz <2433038...@qq.com> Gerrit-Comment-Date: Tue, 22 Aug 2023 11:53:07 +0000 Gerrit-HasComments: Yes
