You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2016/11/29 21:04:57 UTC
[kudu-CR] Update debug partition and row printing
Hello Adar Dembo, Dimitris Tsirogiannis, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5262
to review the following change.
Change subject: Update debug partition and row printing
......................................................................
Update debug partition and row printing
This commit updates the debug printing of rows and partitions in
metrics, log messages, and web UIs. The new format is one we have
settled on after consulting with committers on Apache Impala. In all
cases I think it makes it easier to work with complex partitions. The
most invasive aspect of this change is that string and binary column
values are now printed with quotes, which affects a bunch of otherwise
unrelated tests.
I attempted to minimize as much of the duplication as possible in debug
formatting functions in partition.cc, but there is still a lot of
duplication that was unavoidable (this was true before making this
change as well).
Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_changelist-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/schema-test.cc
M src/kudu/common/types.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-pushdown-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tserver/tablet_server-test.cc
25 files changed, 479 insertions(+), 449 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5262/1
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Update debug partition and row printing
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Update debug partition and row printing
......................................................................
Patch Set 1:
screenshots of updated web UIs: http://imgur.com/a/VNlMA
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Update debug partition and row printing
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Update debug partition and row printing
......................................................................
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5262/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
PS1, Line 585: if (partition.hash_buckets_.size() != hash_bucket_schemas_.size()) {
: return "<hash-partition-error>";
: }
should this be a CHECK? would be indicative of programmer error, right? or perhaps a LOG(DFATAL) at least so we'd catch it in test builds?
Line 621: components.emplace_back(Substitute("<hash-error: $0>", s.ToString()));
same
Line 652: return "<hash-decode-error>";
same
PS1, Line 689: oakc
not following this reference
Line 706: return "<range-key-decode-error>";
same
Line 810: ColumnIdsToColumnNames(schema, hash_bucket_schema.column_ids));
for these HTML output functions, we should be escaping the user-provided column names
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Update debug partition and row printing
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: Update debug partition and row printing
......................................................................
Patch Set 1:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/5262/1/src/kudu/common/partition-test.cc
File src/kudu/common/partition-test.cc:
Line 411: EXPECT_EQ(string("", 0), partitions[0].partition_key_start());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 428: EXPECT_EQ(string("", 0), partitions[2].partition_key_end());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 500: EXPECT_EQ(string("", 0), partitions[0].partition_key_start());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 520: EXPECT_EQ(string("", 0), partitions[2].range_key_end());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 551: EXPECT_EQ(string("", 0), partitions[5].range_key_end());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 582: EXPECT_EQ(string("", 0), partitions[8].range_key_end());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 613: EXPECT_EQ(string("", 0), partitions[11].range_key_end());
> warning: constructor creating an empty string [misc-string-constructor]
Done
Line 615: EXPECT_EQ(string("", 0), partitions[11].partition_key_end());
> warning: constructor creating an empty string [misc-string-constructor]
Done
http://gerrit.cloudera.org:8080/#/c/5262/1/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:
Line 539: for (ColumnId column_id : range_schema_.column_ids) {
> warning: loop variable is copied but only used as const reference; consider
Done
Line 549: for (ColumnId column_id : range_schema_.column_ids) {
> warning: loop variable is copied but only used as const reference; consider
Done
Line 574: for (ColumnId column_id : column_ids) {
> warning: loop variable is copied but only used as const reference; consider
Done
PS1, Line 585: if (partition.hash_buckets_.size() != hash_bucket_schemas_.size()) {
: return "<hash-partition-error>";
: }
> should this be a CHECK? would be indicative of programmer error, right? or
It's not always indicative of a programmer error. These debug print routines are used to construct good error messages in the case of malformed DDL requests. I also found that PartitionPruner calls them with partition keys that aren't necessarily valid according to the schema (but that's not a bug).
Line 621: components.emplace_back(Substitute("<hash-error: $0>", s.ToString()));
> same
Done
Line 652: return "<hash-decode-error>";
> same
Done
Line 681: } else if (lower_unbounded) {
> warning: don't use else after return [readability-else-after-return]
Done
PS1, Line 689: oakc
> not following this reference
Done
Line 706: return "<range-key-decode-error>";
> same
Done
Line 728: } else {
> warning: don't use else after return [readability-else-after-return]
Done
Line 736: for (ColumnId column_id : range_schema_.column_ids) {
> warning: loop variable is copied but only used as const reference; consider
Done
Line 750: } else {
> warning: don't use else after return [readability-else-after-return]
Done
Line 810: ColumnIdsToColumnNames(schema, hash_bucket_schema.column_ids));
> for these HTML output functions, we should be escaping the user-provided co
Done
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Update debug partition and row printing
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5262
to look at the new patch set (#3).
Change subject: Update debug partition and row printing
......................................................................
Update debug partition and row printing
This commit updates the debug printing of rows and partitions in
metrics, log messages, and web UIs. The new format is one we have
settled on after consulting with committers on Apache Impala. In all
cases I think it makes it easier to work with complex partitions. The
most invasive aspect of this change is that string and binary column
values are now printed with quotes, which affects a bunch of otherwise
unrelated tests.
I attempted to minimize as much of the duplication as possible in debug
formatting functions in partition.cc, but there is still a lot of
duplication that was unavoidable (this was true before making this
change as well).
Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_changelist-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/schema-test.cc
M src/kudu/common/types.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-pushdown-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tserver/tablet_server-test.cc
25 files changed, 507 insertions(+), 472 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5262/3
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Update debug partition and row printing
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5262
to look at the new patch set (#2).
Change subject: Update debug partition and row printing
......................................................................
Update debug partition and row printing
This commit updates the debug printing of rows and partitions in
metrics, log messages, and web UIs. The new format is one we have
settled on after consulting with committers on Apache Impala. In all
cases I think it makes it easier to work with complex partitions. The
most invasive aspect of this change is that string and binary column
values are now printed with quotes, which affects a bunch of otherwise
unrelated tests.
I attempted to minimize as much of the duplication as possible in debug
formatting functions in partition.cc, but there is still a lot of
duplication that was unavoidable (this was true before making this
change as well).
Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_changelist-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/schema-test.cc
M src/kudu/common/types.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-pushdown-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tserver/tablet_server-test.cc
25 files changed, 505 insertions(+), 470 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/5262/2
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Update debug partition and row printing
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Update debug partition and row printing
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Update debug partition and row printing
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.
Change subject: Update debug partition and row printing
......................................................................
Update debug partition and row printing
This commit updates the debug printing of rows and partitions in
metrics, log messages, and web UIs. The new format is one we have
settled on after consulting with committers on Apache Impala. In all
cases I think it makes it easier to work with complex partitions. The
most invasive aspect of this change is that string and binary column
values are now printed with quotes, which affects a bunch of otherwise
unrelated tests.
I attempted to minimize as much of the duplication as possible in debug
formatting functions in partition.cc, but there is still a lot of
duplication that was unavoidable (this was true before making this
change as well).
Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Reviewed-on: http://gerrit.cloudera.org:8080/5262
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/common/encoded_key-test.cc
M src/kudu/common/key_util-test.cc
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition.cc
M src/kudu/common/partition.h
M src/kudu/common/row_changelist-test.cc
M src/kudu/common/row_operations-test.cc
M src/kudu/common/scan_spec-test.cc
M src/kudu/common/schema-test.cc
M src/kudu/common/types.h
M src/kudu/master/master-path-handlers.cc
M src/kudu/tablet/compaction-test.cc
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/major_delta_compaction-test.cc
M src/kudu/tablet/memrowset-test.cc
M src/kudu/tablet/tablet-pushdown-test.cc
M src/kudu/tablet/tablet-test-base.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tools/kudu-ts-cli-test.cc
M src/kudu/tserver/tablet_server-test.cc
25 files changed, 507 insertions(+), 472 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4c444b155fe6621af65b86020be105fe56ae18ef
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>