Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/9069 )
Change subject: KUDU-2126: Add conditional check to prevent unnecessary fsyncs ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@9 PS2, Line 9: nit: a unit test http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@11 PS2, Line 11: add nit: adds http://gerrit.cloudera.org:8080/#/c/9069/2//COMMIT_MSG@12 PS2, Line 12: another nit: additional http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc File src/kudu/integration-tests/delete_tablet-itest.cc: http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc@169 PS2, Line 169: KUDU-2126 : Ensure that DeleteTablet RPCs does not do double fsyncs. how about: // Regression test for KUDU-2126: Ensure that a DeleteTablet() RPC on a tombstoned tablet does not cause any fsyncs. http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/integration-tests/delete_tablet-itest.cc@191 PS2, Line 191: ASSERT_EVENTUALLY([&] { Is this test flaky if you remove this ASSERT_EVENTUALLY? I don't think this is required because workload.Setup() should not return until the tablet has been created. http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.h@379 PS2, Line 379: int flush_count_for_tests_; This seems fine, but I wonder if you considered creating a metric for # of fsyncs. http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tablet/tablet_metadata.cc@554 PS2, Line 554: flush_count_for_tests_++; I think this line should go inside TabletMetadata::ReplaceSuperBlockUnlocked() since that is where the actual flush is taking place. The flush occurs during the call to pb_util::WritePBContainerToPath(). http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc@797 PS2, Line 797: KUDU-2126 : i No need to mention KUDU-2126 here. It should be mentioned in the regression test, instead. http://gerrit.cloudera.org:8080/#/c/9069/2/src/kudu/tserver/ts_tablet_manager.cc@798 PS2, Line 798: p style nit: end comments with punctuation (i.e. a period). We follow the Google C++ style guide and this guideline can be found at https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_Grammar -- To view, visit http://gerrit.cloudera.org:8080/9069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idde8b413374f43ca2ef09339f0f412208f03685e Gerrit-Change-Number: 9069 Gerrit-PatchSet: 2 Gerrit-Owner: Jeffrey F. Lukman <[email protected]> Gerrit-Reviewer: Jeffrey F. Lukman <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 20 Jan 2018 01:57:33 +0000 Gerrit-HasComments: Yes
