Michael Ho 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: (11 comments) http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test-base.h@441 PS1, Line 441: FLAGS_rpc_encrypt_loopback_connections = true; > ah, does this mean we were not successfully testing encrypted RPCs in the p I believe so. I confirmed that the TlsSocket path is now being exercised by manually adding a CHECK() in that TlsSocket::Write(). http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@740 PS1, Line 740: // Linux, SSL enabled > hmm, is this fix related to the patch in question? I'm surprised we haven't Yes, as we weren't using TlsSocket before so we weren't triggering this error message. http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1223 PS1, Line 1223: // Helper function for TestCancellationMultiThreads. > better to document briefly what it does, rather than what it is. Done http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1224 PS1, Line 1224: SendRpcAndCancel > this should probably be named SendAndCancelRpcs or something, since it send Done http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1251 PS1, Line 1251: ASSERT_TRUE > have to use CHECK here since this isn't the main thread and gtest isn't thr Done http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1256 PS1, Line 1256: #define BUF_SIZE (16 * 1024 * 1024) > can you use a const int defined below within the function instead? We pref Done http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1258 PS1, Line 1258: Regression > did this test successfully repro the bug w/o the fix? Unfortunately, no even with multiple tries in different build types. That said, I think this is still a good test to include. Any suggestion to make this test more effective in triggering the bug is welcomed. I confirmed the fix with Impala stress test running on a 130 node cluster which hit this bug quite consistently without the fix. Updated the commit message with this piece of information. http://gerrit.cloudera.org:8080/#/c/9587/1/src/kudu/rpc/rpc-test.cc@1274 PS1, Line 1274: unique_ptr<uint8_t[]> buf(new uint8_t[BUF_SIZE]); : memset(buf.get(), 'a', BUF_SIZE); : Slice slice(buf.get(), BUF_SIZE); > you could probably get away with just: Done 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." Done 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 adde Done 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 backfi As far as I can tell, this should be fine. Both SendBuffer() and OutboundTransfer::TransferStarted() are called from Connection::WriteHandler() only. So, if we get to the point of SendBuffer(), we should have taken the path of if (!transfer->TransferStarted()) once. -- 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 <k...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 13 Mar 2018 00:54:24 +0000 Gerrit-HasComments: Yes
