Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8570 )
Change subject: WIP: tls_socket: properly handle temporary socket errors in Writev ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8570/1/src/kudu/security/tls_socket.cc File src/kudu/security/tls_socket.cc: http://gerrit.cloudera.org:8080/#/c/8570/1/src/kudu/security/tls_socket.cc@99 PS1, Line 99: if (total_written > 0 && Socket::IsTemporarySocketError(write_status.posix_code())) { : return Status::OK(); : } > How do you know it will call it with the same length of 10? In other words, I think it's brittle to assume the caller will call it with the same length. The offset in the buffer will be the same, yes, but it's brittle to assume the caller will use the same length for the next call, since that's not in the contract of that OutboundTransfer::SendBuffer API, as I understand. Maybe, we should also enable SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER for the TlsContext. Also, I found that enabling SSL_MODE_ENABLE_PARTIAL_WRITE for TlsContext (without any other changes) fixes the issue with that error in LargeWrites test. But this fix is also needed to have consistent behavior of TlsSocket::Writev(). Overall, I think enabling SSL_MODE_ENABLE_PARTIAL_WRITE might be beneficial to have as well: so far callers of TlsSocket::Write() handle partial writes correctly, and there will be less difference in behavior of TlsSocket::Write() and Socket::Write() in that case. -- To view, visit http://gerrit.cloudera.org:8080/8570 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If797f220f42bfb2e6f452b66f15e7a758e883472 Gerrit-Change-Number: 8570 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Thu, 16 Nov 2017 19:34:36 +0000 Gerrit-HasComments: Yes
