Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11367 )
Change subject: [tablet] Improve logging of maintenance ops ...................................................................... Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/11367/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11367/2//COMMIT_MSG@7 PS2, Line 7: Improve logging of maintenance ops > Could you update this to follow the pattern in most of other Kudu commmits: Done http://gerrit.cloudera.org:8080/#/c/11367/2//COMMIT_MSG@12 PS2, Line 12: MRS flush and rowset compaction logging now includes the number of new rowsets > nit: could you keep these lines no longer than 72 symbols? Other lines wit Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@107 PS2, Line 107: push_back > nit: maybe, use emplace_back(std::move(...)) instead The value being pushed is a temporary of type std::string, so I think it will be moved from by the overload of push_back that takes an rvalue. http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@109 PS2, Line 109: > nit: indent Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@109 PS2, Line 109: push_back > ditto ditto :) http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@296 PS2, Line 296: // Throwaway struct to combine stats from multiple delta stores. : struct MergedDeltaStats { > Do you expect to use this more extensively? Why not just a `string DeltaSto Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@316 PS2, Line 316: > nit: add const specifier N/A see Andrew's comment. http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@330 PS2, Line 330: const shared_ptr<DeltaStore>& > nit: would 'const auto&' work here as well? Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@341 PS2, Line 341: << "Finished major delta compaction of columns " : << ColumnNamesToString() << ". Compacted " << included_stores_.size() : << " delta files. Overall stats: " << mds.ToString(); > nit: consider using strings::Substitute() here Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_stats.h File src/kudu/tablet/delta_stats.h: http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_stats.h@89 PS2, Line 89: int64_t UpdateCount() const; > Maybe, follow the style of already existing update_count_for_col_id() metho I'm following the GSG rule strictly here "Regular functions have mixed case; accessors and mutators may be named like variables." from https://google.github.io/styleguide/cppguide.html#Function_Names. This function isn't an accessor- it does real work iterating over the update_counts_by_col_id_ member- so I named it as a function. http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_tracker.cc@408 PS2, Line 408: LOG_WITH_PREFIX(INFO) << "Flushing compaction of " << compacted_blocks.size() : << " redo delta blocks { " << compacted_blocks : << " } into block " << new_block_id; > I think using strings::Substitute() would help to make it more readable. Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/diskrowset.h@220 PS2, Line 220: int64_t > size() returns size_t; is there any specific reason to convert it into sign To be consistent with the preexisting written_count (which I renamed to row_written_count), and in keeping with the GSG principal of using signed ints a lot (which I know you disagree with). See "On unsigned integers" at https://google.github.io/styleguide/cppguide.html#Integer_Types. http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/tablet.cc@1487 PS2, Line 1487: gced_all_input > If this variable is not used in the code below, consider getting rid of it Done http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/tablet.cc@1651 PS2, Line 1651: << op_name << " successful on " << drsw.rows_written_count() : << " rows " << "(" << drsw.drs_written_count() << " rowsets, " : << drsw.written_size() << " bytes)"; > Maybe, use strings::Substitute() to build this message? That way it will b Done -- To view, visit http://gerrit.cloudera.org:8080/11367 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I43883001c5a1c72ff1ca0c1bc84d24a8533e3891 Gerrit-Change-Number: 11367 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <andrew.w...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 31 Aug 2018 17:10:50 +0000 Gerrit-HasComments: Yes
