You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2022/02/14 22:29:32 UTC
[kudu-CR] [common] add DCHECK to spot bugs in using Schema
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18231
Change subject: [common] add DCHECK to spot bugs in using Schema
......................................................................
[common] add DCHECK to spot bugs in using Schema
While reviewing recently posted patch [1], I was looking at the code in
src/kudu/common/schema.h and decided to add more DCHECK-like asserts
to help catching bugs in the implementation of the Schema class itself
and spotting errors at the call sites where Schema objects are used.
This patch doesn't contain any functional changes.
[1] https://gerrit.cloudera.org/#/c/18213/
Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
---
M src/kudu/common/schema.h
1 file changed, 15 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/18231/1
--
To view, visit http://gerrit.cloudera.org:8080/18231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Gerrit-Change-Number: 18231
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [common] add DCHECK to spot bugs in using Schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18231 )
Change subject: [common] add DCHECK to spot bugs in using Schema
......................................................................
[common] add DCHECK to spot bugs in using Schema
While reviewing recently posted patch [1], I was looking at the code in
src/kudu/common/schema.h and decided to add more DCHECK-like asserts
to help catching bugs in the implementation of the Schema class itself
and spotting errors at the call sites where Schema objects are used.
This patch doesn't contain any functional changes.
[1] https://gerrit.cloudera.org/#/c/18213/
Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Reviewed-on: http://gerrit.cloudera.org:8080/18231
Reviewed-by: Andrew Wong <an...@g.ucla.edu>
Reviewed-by: Mahesh Reddy <mr...@cloudera.com>
Tested-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/common/schema.h
1 file changed, 15 insertions(+), 9 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Mahesh Reddy: Looks good to me, but someone else must approve
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/18231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Gerrit-Change-Number: 18231
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [common] add DCHECK to spot bugs in using Schema
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18231 )
Change subject: [common] add DCHECK to spot bugs in using Schema
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Gerrit-Change-Number: 18231
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 15 Feb 2022 00:00:32 +0000
Gerrit-HasComments: No
[kudu-CR] [common] add DCHECK to spot bugs in using Schema
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18231 )
Change subject: [common] add DCHECK to spot bugs in using Schema
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18231/1/src/kudu/common/schema.h
File src/kudu/common/schema.h:
http://gerrit.cloudera.org:8080/#/c/18231/1/src/kudu/common/schema.h@544
PS1, Line 544: // latter involves division by sizeof(ColumnSchema)
Wow, TIL.
http://gerrit.cloudera.org:8080/#/c/18231/1/src/kudu/common/schema.h@548
PS1, Line 548: DCHECK_EQ(cols_.size(), name_to_index_.size());
nit: in the spirit of catching the problem when it happens, I first thought maybe we ought to put this in the constructors or Reset() calls. OTOH, seems Reset() already has something similar, and it may be a bit uglier spraying this all over the constructors, so feel free to keep this as is.
--
To view, visit http://gerrit.cloudera.org:8080/18231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Gerrit-Change-Number: 18231
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Feb 2022 23:18:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [common] add DCHECK to spot bugs in using Schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [common] add DCHECK to spot bugs in using Schema
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/18231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Gerrit-Change-Number: 18231
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [common] add DCHECK to spot bugs in using Schema
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18231 )
Change subject: [common] add DCHECK to spot bugs in using Schema
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/18231
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I44b73ef06924af6556a7cd3da4a7eec20d12aefc
Gerrit-Change-Number: 18231
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@g.ucla.edu>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jun 2022 20:26:40 +0000
Gerrit-HasComments: No