Alexey Serbin has posted comments on this change. Change subject: KUDU-2125: Tablet copy client does not retry on failures ......................................................................
Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/8016/4//COMMIT_MSG Commit Message: Line 8: Could you elaborate a bit on the way the issue has been addressed by the patch? Also, I think it would be beneficial to add concise description of the problem as well,. http://gerrit.cloudera.org:8080/#/c/8016/4/src/kudu/integration-tests/cluster_itest_util.cc File src/kudu/integration-tests/cluster_itest_util.cc: PS4, Line 854: if (only_running) { nit: does it make to skip this if !s.ok() ? http://gerrit.cloudera.org:8080/#/c/8016/4/src/kudu/integration-tests/tablet_copy_client_session-itest.cc File src/kudu/integration-tests/tablet_copy_client_session-itest.cc: PS4, Line 274: TEST_F(TabletCopyClientSessionITest, TestTabletCopyWithBusySource) How fast does this test run? If it's relatively long, consider running it only if AllowSlowTests() returns true. PS4, Line 305: ASSERT_OK I've heard ASSERT_OK() does not work properly with non-main test threads. If that is still true, consider changing this to CHECK_OK() or exiting from the thread function and then reporting on the failure in the main thread instead. http://gerrit.cloudera.org:8080/#/c/8016/4/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: PS4, Line 669: uint64_t offset = 0; nit: move it under the 'while(!done) {...}' loop scope? PS4, Line 672: FetchDataRequestPB req; nit: move it under the 'while(!done) {...}' loop scope? PS4, Line 702: done = offset + chunk_size >= resp.chunk().total_data_length(); : offset += resp.chunk().data().size(); nit: consider removing the extra '+' operation (at least for clarity) offset += chunk_size; done = offset >= resp.chunk().total_data_length(); Line 736: MonoTime deadline = MonoTime::Now() + MonoDelta::FromMilliseconds(session_idle_timeout_millis_); nit: const? Line 748: double kJitterPct = 0.5; nit: static const? PS4, Line 753: MonoTime::Now() + backoff > deadline Is it worth giving it the last chance if, say, (MonoTime::Now() + MonoDelta::FromMilliseconds(kBackoffBaseMs) < deadline) but (MonoTime::Now() + backoff > deadline) ? -- To view, visit http://gerrit.cloudera.org:8080/8016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7c8454fc600a841bd15306a2b3b06ddf53130be6 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
