Adar Dembo has posted comments on this change. Change subject: KUDU-2118. Fully shut down TabletReplica on delete ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/7883/1/src/kudu/integration-tests/delete_tablet-itest.cc File src/kudu/integration-tests/delete_tablet-itest.cc: Line 127: FLAGS_allow_unsafe_replication_factor = true; Could you doc why it's important to use 2 replicas for this test? PS1, Line 148: memory_order_relaxed Nit: while it might be safe to use 'relaxed' instead of the default of 'seq_cst', if 'seq_cst' is also correct I'd argue that brevity (by using the default value) is more useful in a test than using the most performant but still safe value. PS1, Line 156: ASSERT_EVENTUALLY([&] { : ASSERT_OK(itest::DeleteTablet(ts, tablet_id, TABLET_DATA_DELETED, boost::none, kTimeout)); : }); Could you doc why this needs to be wrapped in ASSERT_EVENTUALLY? I imagine you're not worried about timeouts (since kTimeout could be much much higher), so what's the reason? Line 168: running.store(false, memory_order_relaxed); See above. -- To view, visit http://gerrit.cloudera.org:8080/7883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a0ea46f220a10d4de680fc73d45d14426e0395e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
