Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9587 )
Change subject: KUDU-2334: Fix OutboundTransfer::TransferStarted() to work with SSL_write() ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h File src/kudu/rpc/transfer.h: http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h@186 PS1, Line 186: True if we have tried sending anything in 'payload_slices_' Actually isn't it more like "True if SendBuffer() is called at least once." ? The Writev() function can return without attempting to send anything in some cases. http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.h@187 PS1, Line 187: // no bytes were sent successfully. This is needed as SSL_write() is stateful. nit: Add a reference to this JIRA, else it's hard to understand why we added it. http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.cc File src/kudu/rpc/transfer.cc: http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/transfer.cc@190 PS1, Line 190: started_ = true; Do we know of any cases where setting started_ = true this early can backfire? I looked at the code and it seems fine to me, but I just want to make sure. In some cases, we won't send anything at all: https://github.com/apache/kudu/blob/master/src/kudu/security/tls_socket.cc#L50-L55 Or we could hit errors before actually calling the SSL_write() or send() functions. So we should make sure that having started_ = true for these cases doesn't cause a problem elsewhere in the code. -- To view, visit http://gerrit.cloudera.org:8080/9587 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7ebdcbc1ef2a3e0c5e7162f03214c232755b683 Gerrit-Change-Number: 9587 Gerrit-PatchSet: 1 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 12 Mar 2018 23:36:34 +0000 Gerrit-HasComments: Yes
