Dan Burkert has posted comments on this change. Change subject: KUDU-2125: Tablet copy client does not retry on failures ......................................................................
Patch Set 4: (11 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 pa Done 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() ? Done http://gerrit.cloudera.org:8080/#/c/8016/4/src/kudu/integration-tests/cluster_itest_util.h File src/kudu/integration-tests/cluster_itest_util.h: Line 330: bool only_running = false); > Maybe it'd be better to pass a `boost::optional<TabletStatePB>` instead? Th Done 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 Done PS4, Line 305: ASSERT_OK > I've heard ASSERT_OK() does not work properly with non-main test threads. Doesn't seem to be an issue in practice, is there something in particular to watch out for? Adding an ASSERT_TRUE(false) to the thread routines causes the test to fail with the expected output. 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? The offset is updated each time through the loop. PS4, Line 672: FetchDataRequestPB req; > nit: move it under the 'while(!done) {...}' loop scope? Took this in a bit different direction, but it should make it clear why it's outside the loop. 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) I kept around the addition since I think it makes it more readable to compare to offset + chunk_size, but I did replace the redundant field accesses on 703 with 'chunk_size'. Line 736: MonoTime deadline = MonoTime::Now() + MonoDelta::FromMilliseconds(session_idle_timeout_millis_); > nit: const? Any reason to prefer const here? Line 748: double kJitterPct = 0.5; > nit: static const? This is the only place these constants are used, so I don't see an advantage to making them static. PS4, Line 753: MonoTime::Now() + backoff > deadline > Is it worth giving it the last chance if, say, (MonoTime::Now() + MonoDelta I don't personally think it's worth complicating with a special case. -- 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
