Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20892 )
Change subject: [net] DiagnosticSocket wrapper for sock_diag API ...................................................................... Patch Set 1: (5 comments) Thank you for feedback! http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/diagnostic_socket.h File src/kudu/util/net/diagnostic_socket.h: http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/diagnostic_socket.h@17 PS1, Line 17: #pragma once > nit: leave a blank line Done http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/diagnostic_socket.cc File src/kudu/util/net/diagnostic_socket.cc: http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/diagnostic_socket.cc@75 PS1, Line 75: socket > nit:: It would be better to add "::" prefixes for the system functions, oth Done http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/diagnostic_socket.cc@115 PS1, Line 115: RETURN_NOT_OK(ReceiveResponse(&result)); > I'm just curious why not judge the "result" size like the below? With this method, it's possible to have multiple items in the result because of providing corresponding parameters. You can see how it's used in the DiagnosticSocketTest.SimplePattern's code. http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/socket-test.cc File src/kudu/util/net/socket-test.cc: http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/socket-test.cc@247 PS1, Line 247: s.Release(); > Is it necessary to call Release manually? Not exactly necessary. I removed this. http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/socket.cc File src/kudu/util/net/socket.cc: http://gerrit.cloudera.org:8080/#/c/20892/1/src/kudu/util/net/socket.cc@245 PS1, Line 245: false > Is it possible the 'fd' is an already listened fd? Yep, that's a good point. It seems these changes were made in a mechanical way. And frankly, these code had a smell (faint one, though). After some consideration, I decided to remove this 'IsListened' and rely on GetPeerAddress() instead -- that seems to be cleaner. -- To view, visit http://gerrit.cloudera.org:8080/20892 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4a4f37ca4b0df8ca543ec383d89766cbf1b1e526 Gerrit-Change-Number: 20892 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 16 Jan 2024 17:27:00 +0000 Gerrit-HasComments: Yes
