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

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool 

Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11260


Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 13 insertions(+), 8 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/tool_action_pbc.cc@214
PS3, Line 214:   // We successfully wrote the new file. Move the old file to a backup location,
nit: I think this comment should be split into two and the backup part moved under if (FLAGS_backup)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sun, 19 Aug 2018 15:32:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

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

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

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 19 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

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

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

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 50 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11260/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc@863
PS4, Line 863:   // Test 'edit' by setting up EDITOR to be a sed script which performs a substitution.
> Nit: Make sure
Done


http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc@866
PS4, Line 866:     ASSERT_OK(DoEdit("exec sed -i -e s/Formatted/Edited/ \"$@\"\n", &stdout, nullptr,
> Nit: const auto&
Done


http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc@877
PS4, Line 877:     bool found_backup = false;
> You could decompose this into a short helper that handles L846-869 too. Som
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 19:31:01 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11260/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/5/src/kudu/tools/kudu-tool-test.cc@365
PS5, Line 365:     *found = false;
Nit: we try to stick to the calling convention that if a function returns non-OK, we don't modify its OUT parameters.


http://gerrit.cloudera.org:8080/#/c/11260/5/src/kudu/tools/kudu-tool-test.cc@877
PS5, Line 877:     bool found_backup = false;
No need to initialize this or on L889; HasAtLeastOneBackupFile guarantees that it is set if returning OK.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 19:38:21 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

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

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

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 46 insertions(+), 12 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc@845
PS1, Line 845:     return RunTool(Substitute("pbc edit --nobackup $0/instance", kTestDir),
> Done
What about the other half (i.e. that --backup DOES produce a backup file?)


http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc@863
PS3, Line 863: auto &child
Nit: const auto&



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 22:10:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

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

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

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 17 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

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

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

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 50 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/11260/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11260
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................

KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

Adds a -nobackup flag to the pbc edit tool to
simplify usage when the backup file will
not be used and the user doesn’t want to worry about
cleanup.

Change-Id: If544f7682a30933077db0824452639a68507ba40
Reviewed-on: http://gerrit.cloudera.org:8080/11260
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_pbc.cc
2 files changed, 50 insertions(+), 12 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Grant Henke: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11260/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/5/src/kudu/tools/kudu-tool-test.cc@365
PS5, Line 365:     *found = false;
> Nit: we try to stick to the calling convention that if a function returns n
Done


http://gerrit.cloudera.org:8080/#/c/11260/5/src/kudu/tools/kudu-tool-test.cc@877
PS5, Line 877:     bool found_backup = false;
> No need to initialize this or on L889; HasAtLeastOneBackupFile guarantees t
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 19:57:55 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc@845
PS1, Line 845:     setenv("EDITOR", editor_path.c_str(), /* overwrite */1);
> What about the other half (i.e. that --backup DOES produce a backup file?)
Done


http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc@863
PS3, Line 863: ke no backu
> Nit: const auto&
Done


http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc@864
PS3, Line 864: 
> nit: kBakInfix
In this case I actually want the test to fail if we change that given it's a sort of "public api".


http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/tool_action_pbc.cc
File src/kudu/tools/tool_action_pbc.cc:

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/tool_action_pbc.cc@214
PS3, Line 214:   // We successfully wrote the new file.
> nit: I think this comment should be split into two and the backup part move
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 16:13:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 7: Code-Review+2

Carrying the +2 through the rebase.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 21 Aug 2018 03:02:11 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc@845
PS1, Line 845:     return RunTool(Substitute("pbc edit --nobackup $0/instance", kTestDir),
Could you actually verify that --nobackup generates no backup file, while --backup (the default) does? You could probably do it roughly via Env::GetChildren and counting the number of files.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 21:03:39 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/1/src/kudu/tools/kudu-tool-test.cc@845
PS1, Line 845:     return RunTool(Substitute("pbc edit --nobackup $0/instance", kTestDir),
> Could you actually verify that --nobackup generates no backup file, while -
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 22:02:46 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc@863
PS4, Line 863:     // Make no backup file was written.
Nit: Make sure


http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc@866
PS4, Line 866:     for (const auto &child : children) {
Nit: const auto&

(Yeah that's what I wrote earlier, but I was trying to draw attention not just to the lack of const, but also the placement of the ampersand next to the word 'auto').


http://gerrit.cloudera.org:8080/#/c/11260/4/src/kudu/tools/kudu-tool-test.cc@877
PS4, Line 877:     // Make sure a backup file was written.
You could decompose this into a short helper that handles L846-869 too. Something like:

  Status HasAtLeastOneBackupFile(const string& dir, bool* found)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 18:53:47 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 22:32:37 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 21:34:28 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool

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

Change subject: KUDU-2531: (part 2) Add -nobackup flag to pbc edit tool
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/11260/3/src/kudu/tools/kudu-tool-test.cc@864
PS3, Line 864: ".bak"
nit: kBakInfix



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If544f7682a30933077db0824452639a68507ba40
Gerrit-Change-Number: 11260
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 18 Aug 2018 00:18:25 +0000
Gerrit-HasComments: Yes