Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11248 )
Change subject: KUDU-2469 pt 1: add an IOContext ...................................................................... Patch Set 3: (19 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 produc As discussed on Slack, I'm going to do this in the next patch that actually dereferences the IOContext*s. 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) + init_once_.memory_footprint_excluding_this(); > And here? Done 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/gutil/gscoped_ptr.h" > Couldn't you forward declare IOContext, since you're passing it by pointer? Yeah, when I first started plumbing, I thought I was being clever by using move semantics everywhere, but I realized pretty quickly that that wouldn't cut it. http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/cfile/cfile_reader.h@101 PS2, Line 101: > Seems like a good candidate for adding to IOContext. May want to wait until Ack 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? Ah, fallout from me thinking the new init_once didn't need a `memory_footprint_excluding_this()`. I realized the err, but didn't revert this. http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/delta_tracker.h@157 PS2, Line 157: // Updates the in-memory list of delta stores and then persists the updated > warning: missing username/bug in TODO [google-readability-todo] I think this is old enough to remove, considering we have maintenance op scheduling based on stats. Same with the one in Flush() http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/delta_tracker.h@310 PS2, Line 310: > warning: function 'kudu::tablet::DeltaTracker::MakeDeltaIteratorMergerUnloc Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/delta_tracker.h@310 PS2, Line 310: > warning: function 'kudu::tablet::DeltaTracker::MakeDeltaIteratorMergerUnloc Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/local_tablet_writer.h File src/kudu/tablet/local_tablet_writer.h: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/local_tablet_writer.h@27 PS2, Line 27: #include "kudu/tablet/tablet.h" > warning: #includes are not sorted properly [llvm-include-order] Done 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? Ah hm, I guess not? I'd always been following the examples presented in the GSG, though I don't see explicit guidance for the spacing: https://google.github.io/styleguide/cppguide.html#Namespaces http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/mock-rowsets.h File src/kudu/tablet/mock-rowsets.h: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/mock-rowsets.h@43 PS2, Line 43: bool* /*present*/, ProbeStats* /*stats*/) const OVERRIDE { > warning: parameter 'stats' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/mock-rowsets.h@52 PS2, Line 52: ProbeStats* /*stats*/, > warning: parameter 'stats' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/mock-rowsets.h@53 PS2, Line 53: OperationResultPB* /*result*/) OVERRIDE { > warning: parameter 'result' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/mock-rowsets.h@65 PS2, Line 65: gscoped_ptr<CompactionInput>* /*out*/) const OVERRIDE { > warning: parameter 'out' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/mock-rowsets.h@69 PS2, Line 69: virtual Status CountRows(const fs::IOContext* /*io_context*/, rowid_t* /*count*/) const OVERRIDE { > warning: parameter 'count' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/rowset.cc File src/kudu/tablet/rowset.cc: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/rowset.cc@119 PS2, Line 119: gscoped_ptr<CompactionInput>* /*out*/) const { > warning: parameter 'out' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/tablet.h@774 PS2, Line 774: const OrderMode order, fs::IOContext io_context); > warning: parameter 'order' is const-qualified in the function declaration; I'm not sure I understand this, it's `const` in both the declaration and the definition. http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/11248/2/src/kudu/tablet/tablet.cc@1773 PS2, Line 1773: // TODO(todd): support open-ended intervals > warning: missing username/bug in TODO [google-readability-todo] Ack 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: RETURN_NOT_OK_PREPEND(tablet_->AcquireRowLocks(tx_state), > Seems like this should be plumbed in from a higher level in the call stack Done -- 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: 3 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: Sat, 25 Aug 2018 04:03:35 +0000 Gerrit-HasComments: Yes
