David Ribeiro Alves has posted comments on this change. ( http://gerrit.cloudera.org:8080/8388 )
Change subject: exactly_once_writes-itest for faulty disks ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc File src/kudu/integration-tests/exactly_once_writes-itest.cc: http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@77 PS8, Line 77: using cluster::ExternalTabletServer; nit: move before the two above http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@83 PS8, Line 83: int64_t xs nit: why is this the only int64_t ? doesn't seem like it would be feasible to have over 2B attempts http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@110 PS8, Line 110: If 'allow_crashes' is set, crashed servers are restarted. maybe add that if 'allow_crashes' is false the test is caused to fail (that is what happens, right?) http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@118 PS8, Line 118: matching commit indexes. not sure what this means... http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@234 PS8, Line 234: { : std::lock_guard<simple_spinlock> l(lock); : if (num_complete == num_threads) break; : } : if (allow_crashes) { : RETURN_IF_FATALS(RestartAnyCrashedTabletServers(), : "Failed to restart crashed tablet servers"); : } else { : RETURN_IF_FATALS(AssertNoTabletServersCrashed(), : "Some tablet servers crashed"); : } : SleepFor(MonoDelta::FromMilliseconds(10)); nit: might be slightly cleaner to use a countdown latch (avoid the extra synchronization around the shared var and has a timeout function that replaces the sleep) http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/integration-tests/exactly_once_writes-itest.cc@303 PS8, Line 303: LOG(INFO) debug? same below http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/util/test_macros.h File src/kudu/util/test_macros.h: http://gerrit.cloudera.org:8080/#/c/8388/8/src/kudu/util/test_macros.h@27 PS8, Line 27: // Sometimes it's useful to return a status, so we're not forced to write void : // functions to check for fatals explain this better, can't figure it out just from reading this without much more context -- To view, visit http://gerrit.cloudera.org:8080/8388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I966b41b71e39827f8f78fcca59a2208f13f71e53 Gerrit-Change-Number: 8388 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 24 Jul 2018 21:14:31 +0000 Gerrit-HasComments: Yes
