Todd Lipcon 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:

(8 comments)

> Patch Set 1:
>
> the test failure seems to be infrastructure issue ?
>
> tablet-decoder-eval-test.0    dist-test-slave-dist-test-slave-p2pc    -2      
> None            failed to download task files: WARNING 100 
> isolateserver(1484): Adding unknown file 9a0053f2a34

yea that's odd, agree it's unrelated.

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 past 
with this test? :(


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 
seen this failure on our flaky test dashboard


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.


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 sends 
and cancels more than one of them


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 
thread-safe


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 prefer 
'const int kBufferSize' vs a #define


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?


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:

  string buf('a', kBufferSize);

and then down below pass Slice(buf)



--
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: Mon, 12 Mar 2018 22:59:10 +0000
Gerrit-HasComments: Yes

Reply via email to