Alexey Serbin has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired ......................................................................
Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/6648/2/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: Line 80: token_validity_seconds_(2), > is this going to be flaky in TSAN? or is it OK because the new behavior wil I changed num_tablet_servers_ to 3 to comply with the restrictions in our code to use TestWorkload for better test coverage. As I see, the heartbeat interval as 10 msec is successfully used in majority of generic Kudu tests and it does not cause flakiness. Short token validity interval should not bring problems here because the client is supposed to retry, so I don't expect flakiness in SAN builds, given that 2 seconds is enough to complete Kudu RPC eventually, even on slow VMs. PS2, Line 78: : num_tablet_servers_(2), : heartbeat_interval_ms_(10), : token_validity_seconds_(2), : tsk_rotation_seconds_(1), : key_column_name_("key"), : key_split_value_(8) { > why are all of these members instead of just constants? Because they are re-used in the code below. Yes, the tsk_rotation_seconds_ become obsolete -- I removed it. Line 88: //FLAGS_scanner_gc_check_interval_us = 50 * 1000; > ? Done Line 98: CHECK_OK(b.Build(&schema_)); > if you don't care about the schema in particular, can you use one of the ex Done PS2, Line 166: size_t > prefer signed type (int) Done PS2, Line 177: NO > typo Done PS2, Line 213: // Since the authn token is verified upon connection establishment, it's : // necessary to make sure the client does not keep the connections to : // corresponding tablet servers open. So, let's restart the tablet servers : // since the RPC layer might keep already established connections open. > we could set the rpc keepalive timer to short, instead? Good idea -- I added that. Line 229: //ASSERT_OK(scanner.SetFaultTolerant()); > ? Done Line 230: ASSERT_OK(scanner.Open()); > this is only really testing the case of the token being expired on the begi Good idea -- I added that for connection negotiations which is just cover some edge-cases of the authn token re-acq logic. I'll extend this to other RPCs as well. PS2, Line 230: ASSERT_OK(scanner.Open()); : size_t row_count = 0; : while (scanner.HasMoreRows()) { : vector<KuduRowResult> rows; : ASSERT_OK(scanner.NextBatch(&rows)); : row_count += rows.size(); : } : EXPECT_EQ(2, row_count); > I think client-test-util has some code to CountRowsFromScanner or something Thanks, done. -- To view, visit http://gerrit.cloudera.org:8080/6648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
