[kudu-CR] Improve logging of maintenance ops
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/11367 )
Change subject: Improve logging of maintenance ops
..
Patch Set 2:
(1 comment)
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@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
DeltaStoreStatsToString(vector)`?
--
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Fri, 31 Aug 2018 05:15:41 +
Gerrit-HasComments: Yes
[kudu-CR] Improve logging of maintenance ops
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11367 )
Change subject: Improve logging of maintenance ops
..
Patch Set 2:
(13 comments)
looks good, just some nits
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:
[tablet] improve logging of maintenance ops
?
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 with
examples is OK to be as as, but here it would be nice to adhere to Kudu commit
guidelines.
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
http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@109
PS2, Line 109:
nit: indent
http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@109
PS2, Line 109: push_back
ditto
http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@316
PS2, Line 316:
nit: add const specifier
http://gerrit.cloudera.org:8080/#/c/11367/2/src/kudu/tablet/delta_compaction.cc@330
PS2, Line 330: const shared_ptr&
nit: would 'const auto&' work here as well?
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
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() method
and call this
update_count()
or
update_count_total()
or
update_count_all_columns() ?
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.
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 signed
integer?
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 and
just adding explanatory comment about drsw.rows_written_count() == 0 meaning.
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 be
easier to read in the code.
--
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Comment-Date: Fri, 31 Aug 2018 04:52:08 +
Gerrit-HasComments: Yes
[kudu-CR] Improve logging of maintenance ops
Hello Mike Percy, Alexey Serbin, Andrew Wong, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11367 to look at the new patch set (#2). Change subject: Improve logging of maintenance ops .. Improve logging of maintenance ops MRS flushes and rowset compactions = MRS flush and rowset compaction logging now includes the number of new rowsets flushed. Before: I0830 14:54:24.353442 2830984064 tablet.cc:1651] T test_tablet_id P 78c16245dcd84048bf66debf1958c169: Flush successful on 100 rows (9272 bytes) I0830 14:58:23.309068 2830984064 tablet.cc:1651] T test_tablet_id P 15853c32ec1d4f70b82aef90da2108a6: Compaction successful on 30 rows (4957 bytes) After: I0830 14:54:24.353442 2830984064 tablet.cc:1651] T test_tablet_id P 78c16245dcd84048bf66debf1958c169: Flush successful on 100 rows (1 rowsets, 9272 bytes) I0830 14:58:23.309068 2830984064 tablet.cc:1651] T test_tablet_id P 15853c32ec1d4f70b82aef90da2108a6: Compaction successful on 30 rows (1 rowsets, 4957 bytes) Major delta compactions === Major delta compaction had logging that was a little too verbose- for each delta store compacted, it printed out a separate log message with mutation counts. Instead, this patch makes the old more detailed output appear only when verbose logging is on and at INFO level substitutes a total count of each mutation type and a count of delta files compacted, as part of the "Finished" message. Before: I0830 15:22:05.918965 2830984064 delta_compaction.cc:296] Starting major delta compaction for columns val1[int32 NOT NULL] val3[int32 NOT NULL] val4[string NOT NULL] I0830 15:22:05.919018 2830984064 delta_compaction.cc:300] Preparing to major compact delta file: 0145461451762035 (ts range=[101, 150], delete_count=[0], reinsert_count=[0], update_counts_by_col_id=[11:50,13:50,14:50]) I0830 15:22:05.919056 2830984064 delta_compaction.cc:300] Preparing to major compact delta file: 0145461451762036 (ts range=[151, 200], delete_count=[0], reinsert_count=[0], update_counts_by_col_id=[11:50,13:50,14:50]) I0830 15:22:05.921931 2830984064 delta_compaction.cc:306] Finished major delta compaction of columns val1[int32 NOT NULL] val3[int32 NOT NULL] val4[string NOT NULL] After: I0830 15:28:02.737797 2830984064 delta_compaction.cc:326] Starting major delta compaction for columns val1[int32 NOT NULL], val3[int32 NOT NULL], val4[string NOT NULL] I0830 15:28:02.740360 2830984064 delta_compaction.cc:341] Finished major delta compaction of columns val1[int32 NOT NULL], val3[int32 NOT NULL], val4[string NOT NULL]. Compacted 2 delta files. Overall stats: delete_count=0, reinsert_count=0, update_count=300 Minor delta compactions and deltamemstore flushes already include good information on how much was compacted and flushed, respectively. Their logging is unchanged. Change-Id: I43883001c5a1c72ff1ca0c1bc84d24a8533e3891 --- M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/delta_stats.cc M src/kudu/tablet/delta_stats.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/tablet.cc 6 files changed, 64 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/11367/2 -- 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: newpatchset Gerrit-Change-Id: I43883001c5a1c72ff1ca0c1bc84d24a8533e3891 Gerrit-Change-Number: 11367 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] Improve logging of maintenance ops
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11367 Change subject: Improve logging of maintenance ops .. Improve logging of maintenance ops MRS flushes and rowset compactions = MRS flush and rowset compaction logging now includes the number of new rowsets flushed. Before: I0830 14:54:24.353442 2830984064 tablet.cc:1651] T test_tablet_id P 78c16245dcd84048bf66debf1958c169: Flush successful on 100 rows (9272 bytes) I0830 14:58:23.309068 2830984064 tablet.cc:1651] T test_tablet_id P 15853c32ec1d4f70b82aef90da2108a6: Compaction successful on 30 rows (4957 bytes) After: I0830 14:54:24.353442 2830984064 tablet.cc:1651] T test_tablet_id P 78c16245dcd84048bf66debf1958c169: Flush successful on 100 rows (1 rowsets, 9272 bytes) I0830 14:58:23.309068 2830984064 tablet.cc:1651] T test_tablet_id P 15853c32ec1d4f70b82aef90da2108a6: Compaction successful on 30 rows (1 rowsets, 4957 bytes) Major delta compactions === Major delta compaction had logging that was a little too verbose- for each delta store compacted, it printed out a separate log message with mutation counts. Instead, this patch makes the old more detailed output appear only when verbose logging is on and at INFO level substitutes a total count of each mutation type and a count of delta files compacted, as part of the "Finished" message. Before: I0830 15:09:31.731609 2830984064 delta_compaction.cc:296] Starting major delta compaction for columns val1[int32 NOT NULL] val3[int32 NOT NULL] val4[string NOT NULL] I0830 15:09:31.731645 2830984064 delta_compaction.cc:300] Preparing to major compact delta file: 008509586267 (ts range=[101, 150], delete_count=[0], reinsert_count=[0], update_counts_by_col_id=[11:50,13:50,14:50]) I0830 15:09:31.731672 2830984064 delta_compaction.cc:300] Preparing to major compact delta file: 008509586268 (ts range=[151, 200], delete_count=[0], reinsert_count=[0], update_counts_by_col_id=[11:50,13:50,14:50]) I0830 15:09:31.735069 2830984064 delta_compaction.cc:306] Finished major delta compaction of columns val1[int32 NOT NULL] val3[int32 NOT NULL] val4[string NOT NULL] After: I0830 14:45:38.433037 2830984064 delta_compaction.cc:326] Starting major delta compaction for columns val1[int32 NOT NULL], val3[int32 NOT NULL], val4[string NOT NULL] I0830 14:45:38.435420 2830984064 delta_compaction.cc:341] Finished major delta compaction of columns val1[int32 NOT NULL], val3[int32 NOT NULL], val4[string NOT NULL]. Compacted 1 delta files. Overall stats: delete_count=0, reinsert_count=0, update_count=150 Minor delta compactions and deltamemstore flushes already include good information on how much was compacted and flushed, respectively. Their logging is unchanged. Change-Id: I43883001c5a1c72ff1ca0c1bc84d24a8533e3891 --- M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/delta_stats.cc M src/kudu/tablet/delta_stats.h M src/kudu/tablet/delta_tracker.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/tablet.cc 6 files changed, 64 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/11367/1 -- 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: newchange Gerrit-Change-Id: I43883001c5a1c72ff1ca0c1bc84d24a8533e3891 Gerrit-Change-Number: 11367 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
