Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11248 )
Change subject: KUDU-2469 pt 1: add an IOContext ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/bloomfile-test.cc File src/kudu/cfile/bloomfile-test.cc: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/bloomfile-test.cc@58 PS2, Line 58: ASSERT_OK_FAST(bfr_->CheckKeyPresent(BloomKeyProbe(s), nullptr, &present)); What do you think about asserting that the IOContext exists in every production method that expects it? It means more overhead for tests, but it'll mean we needn't sprinkle "if (io_context)" checks in production code, and our test coverage will tell us if we've plumbed a nullptr instead of a real IOContext into some method (instead of silently omitting whatever handling was supposed to happen). http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/bloomfile.cc File src/kudu/cfile/bloomfile.cc: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/bloomfile.cc@344 PS2, Line 344: return kudu_malloc_usable_size(this); And here? http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/cfile_reader.h File src/kudu/cfile/cfile_reader.h: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/cfile_reader.h@38 PS2, Line 38: #include "kudu/fs/io_context.h" Couldn't you forward declare IOContext, since you're passing it by pointer? Maybe IWYU recommended that already. http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/cfile_reader.h@101 PS2, Line 101: CacheControl cache_control, Seems like a good candidate for adding to IOContext. May want to wait until after the dust settles, though. http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/cfile_reader.cc@a582 PS2, Line 582: What happened here? http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/memrowset.h@65 PS2, Line 65: } // namespace tablet Are we actually doing this two space thing? $ git grep '} // namespace' | wc -l 307 $ git grep '} // namespace' | wc -l 1826 http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/tablet_bootstrap.cc@1494 PS2, Line 1494: IOContext io_context({ tablet_meta_->tablet_id() }); Seems like this should be plumbed in from a higher level in the call stack to more precisely indicate the originator of the IO, but that doesn't matter at the moment (since tablet_id is the only member). -- To view, visit http://gerrit.cloudera.org:8080/11248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I645e10a2fda1f2564326b7052e6a0debc5ad738c Gerrit-Change-Number: 11248 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 24 Aug 2018 17:57:25 +0000 Gerrit-HasComments: Yes
