You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/10/08 07:20:02 UTC

[kudu-CR] Cleanup in rowset SVG layout dumping 

Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11612


Change subject: Cleanup in rowset SVG layout dumping
......................................................................

Cleanup in rowset SVG layout dumping

I tested this by manually verifying that the /tablet-rowsetlayout-svg
endpoint and rowset layout SVG file dumping worked as expected.

Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
---
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/svg_dump.cc
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet.cc
4 files changed, 90 insertions(+), 103 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/11612/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11612

to look at the new patch set (#2).

Change subject: Cleanup in rowset SVG layout dumping
......................................................................

Cleanup in rowset SVG layout dumping

I tested this by manually verifying that the /tablet-rowsetlayout-svg
endpoint and rowset layout SVG file dumping worked as expected.

Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
---
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/svg_dump.cc
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet.cc
4 files changed, 92 insertions(+), 105 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/11612/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc
File src/kudu/tablet/svg_dump.cc:

http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@a73
PS3, Line 73: 
            : 
> nit: why to drop those comments?  Are those wrong or outdated?
I just felt they didn't say any more than the code itself.


http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@a70
PS3, Line 70: 
            : 
            : 
            : 
            : 
> nit: let's keep these
Done


http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@a89
PS3, Line 89: 
            : 
> nit: and these
Done


http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@147
PS3, Line 147: }
             : 
             : void PrintXMLHeader(ostream* out) {
             :   CHECK(out);
             :   CHECK(out->go
> nit: maybe, convert this into a multi-line snippet?
What do you mean by that? If you extend a raw literal over multiple lines, you cannot use indentation in later lines because it will be interpreted as part of the literal. That makes splitting up the literal much superior.

Do you mean replacing the sequence of literals and endl's joined by <<'s with literals including \n's? I don't see how that is preferable (it's not really worse, just not worth changing).



-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 05:43:07 +0000
Gerrit-HasComments: Yes

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 4: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 12:10:12 +0000
Gerrit-HasComments: No

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 3: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 21:12:04 +0000
Gerrit-HasComments: No

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc
File src/kudu/tablet/svg_dump.cc:

http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@a70
PS3, Line 70: 
            : 
            : 
            : 
            : 
nit: let's keep these


http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@a89
PS3, Line 89: 
            : 
nit: and these



-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 22:27:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 06:46:11 +0000
Gerrit-HasComments: No

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Fengling Wang, Alexey Serbin, Attila Bukor, Andrew Wong, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11612

to look at the new patch set (#4).

Change subject: Cleanup in rowset SVG layout dumping
......................................................................

Cleanup in rowset SVG layout dumping

I tested this by manually verifying that the /tablet-rowsetlayout-svg
endpoint and rowset layout SVG file dumping worked as expected.

Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
---
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/svg_dump.cc
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet.cc
4 files changed, 95 insertions(+), 104 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/11612/4
-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................

Cleanup in rowset SVG layout dumping

I tested this by manually verifying that the /tablet-rowsetlayout-svg
endpoint and rowset layout SVG file dumping worked as expected.

Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Reviewed-on: http://gerrit.cloudera.org:8080/11612
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/svg_dump.cc
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet.cc
4 files changed, 95 insertions(+), 104 deletions(-)

Approvals:
  Andrew Wong: Looks good to me, approved
  Attila Bukor: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 2: Code-Review+1

Seems there's an IWYU issue, but otherwise LGTM


-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 08 Oct 2018 19:03:10 +0000
Gerrit-HasComments: No

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11612 )

Change subject: Cleanup in rowset SVG layout dumping
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc
File src/kudu/tablet/svg_dump.cc:

http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@a73
PS3, Line 73: 
            : 
nit: why to drop those comments?  Are those wrong or outdated?


http://gerrit.cloudera.org:8080/#/c/11612/3/src/kudu/tablet/svg_dump.cc@147
PS3, Line 147:   *out << R"(<?xml version="1.0" standalone="no"?>)"
             :        << endl
             :        << R"(<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" )"
             :        << R"("http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">)"
             :        << endl;
nit: maybe, convert this into a multi-line snippet?



-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 22:23:13 +0000
Gerrit-HasComments: Yes

[kudu-CR] Cleanup in rowset SVG layout dumping

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Fengling Wang, Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11612

to look at the new patch set (#3).

Change subject: Cleanup in rowset SVG layout dumping
......................................................................

Cleanup in rowset SVG layout dumping

I tested this by manually verifying that the /tablet-rowsetlayout-svg
endpoint and rowset layout SVG file dumping worked as expected.

Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
---
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/svg_dump.cc
M src/kudu/tablet/svg_dump.h
M src/kudu/tablet/tablet.cc
4 files changed, 92 insertions(+), 106 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/11612/3
-- 
To view, visit http://gerrit.cloudera.org:8080/11612
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I521fadf4336f9e11ea611b022cd4d8fcb269dfe2
Gerrit-Change-Number: 11612
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>