You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yifan Zhang (Code Review)" <ge...@cloudera.org> on 2023/01/11 06:09:13 UTC

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Yifan Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19411


Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow for
deleting multiple tablets by table name. Sometimes it is not
convenient for operators to specify multiple tablet ids, but table
names will be much easier to get.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test was added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 92 insertions(+), 6 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 1
Gerrit-Owner: Yifan Zhang <ch...@163.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Ashwani Raina, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow deletion
multiple tablets by table name. Sometimes it is not convenient for
users to specify multiple tablet ids, but table names will be much
easier to get.

In order to support delete all tablets of specified tables, the
<tablet_ids> argument is updated to <tablet_id_patterns> which
supports basic glob syntax. As a result, now we cannot tell users
whether they specified invalid tablet id patterns, the non-exist
tablet id patterns will be ignored even though --ignore_nonexistent
is false.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test is added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 149 insertions(+), 17 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 6
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow for
deleting multiple tablets by table name. Sometimes it is not
convenient for operators to specify multiple tablet ids, but table
names will be much easier to get.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test was added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 101 insertions(+), 17 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/19411/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19411/3//COMMIT_MSG@9
PS3, Line 9: allow deletion
           : multiple tablets by table
> nit: allow deletion of multiple tablets
Done


http://gerrit.cloudera.org:8080/#/c/19411/3//COMMIT_MSG@11
PS3, Line 11: y multipl
> nit: users
Done


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

http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/kudu-tool-test.cc@4208
PS3, Line 4208: specified tablet id does not exist");
> If tabletid is specified and should work just like a pattern, it would make
Done


http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/kudu-tool-test.cc@4329
PS3, Line 4329:     ASSERT_EQ(kDefaultTableName, tablet_replicas[0]->tablet()->metadata()->table_name());
> nit: Also check the left replica's table name is not kTableName?
Done


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

http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/tool_action_local_replica.cc@801
PS3, Line 801:   if (tablet_ids_str.find('*') == string::npos && uniq_count > tablets_to_delete.size()) {
> Because the input is pattern now, one pattern may match multiple tablet ids
Yes. So with this change, we can only identify wrong tablet ids but not wrong tablet id patterns.


http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/tool_action_local_replica.cc@1392
PS3, Line 1392: AddOptionalParameter("tables")
> I am not sure if I get this. Where are we storing table name from user in c
It is stored in gflag value.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 07:07:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 7: Verified+1

Unrelated test failure in TsTabletManagerITest.TestTableStats(TSAN).


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 7
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Jan 2023 03:09:11 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Ashwani Raina, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow deletion
multiple tablets by table name. Sometimes it is not convenient for
users to specify multiple tablet ids, but table names will be much
easier to get.

In order to support delete all tablets of specified tables, the
<tablet_ids> argument is updated to <tablet_id_patterns> which
support basic glob syntax. As a result, now we can not tell users
whether there is an illegal tablet id pattern well and truly, unless
all the specified tablet id patterns are tablet ids.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test is added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 102 insertions(+), 17 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 5
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19411/5/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

http://gerrit.cloudera.org:8080/#/c/19411/5/src/kudu/tools/tool_action_local_replica.cc@793
PS5, Line 793: GetTabletIdsByTableName
> nit: It would worth finding out how this performs on scaled configuration w
I think there shouldn't be too much overhead in FsManager::ListTabletIds and TabletMetadata::Load since we already open the tablet server's metadata at L790.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 6
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 06:49:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 3:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/kudu-tool-test.cc@4329
PS3, Line 4329:   }
nit: Also check the left replica's table name is not kTableName?


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

http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/tool_action_local_replica.cc@801
PS3, Line 801:   if (uniq_count > tablets_to_delete.size()) {
Because the input is pattern now, one pattern may match multiple tablet ids, and some pattern may match none tablet id, so even if uniq_count <= tablets_to_delete.size(), there maybe some patterns are ignored. right?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 12 Jan 2023 13:11:54 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Ashwani Raina, Yingchun Lai, Kudu Jenkins, Ádám Bakai, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow deleting
multiple tablets by table name. Sometimes it is not convenient for
users to specify multiple tablet ids, but table names will be much
easier to get.

In order to support deleting all tablets of specified tables, the
<tablet_ids> argument is updated to <tablet_id_patterns> which
supports basic glob syntax.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag was not specified.

A new test is added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 207 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/19411/8
-- 
To view, visit http://gerrit.cloudera.org:8080/19411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 8
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 8
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Sun, 22 Jan 2023 07:45:00 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19411/6/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

http://gerrit.cloudera.org:8080/#/c/19411/6/src/kudu/tools/tool_action_local_replica.cc@801
PS6, Line 801: 
> I think this condition at least needs some rework.I tried:
That makes sense. Now the tool returns the same error message for invalid tablet ids and invalid tablet id patterns.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 7
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jan 2023 05:21:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG@16
PS5, Line 16:  cannot 
> Do you mean "cannot" ?
Done


http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG@16
PS5, Line 16: support
> nit: supports
Done


http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG@17
PS5, Line 17: whether they specified invalid tablet id patterns, the non-exist
            : tablet id patterns will be ignored even though --ignore_nonexistent
            : 
> Do you mean tablet id patterns will be treated as invalid?
I mean that non-exist tablet id patterns will be ignored even though the -ignore_nonexistent flag is false. But the non-exist tablet ids will not.


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

http://gerrit.cloudera.org:8080/#/c/19411/5/src/kudu/tools/kudu-tool-test.cc@4320
PS5, Line 4320: --tables=$1
> If multiple tables can be entered by user, could you add another test case 
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 6
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Jan 2023 03:58:19 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 7
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Ashwani Raina, Yingchun Lai, Kudu Jenkins, Ádám Bakai, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow deletion
multiple tablets by table name. Sometimes it is not convenient for
users to specify multiple tablet ids, but table names will be much
easier to get.

In order to support delete all tablets of specified tables, the
<tablet_ids> argument is updated to <tablet_id_patterns> which
supports basic glob syntax.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test is added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 206 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/11/19411/7
-- 
To view, visit http://gerrit.cloudera.org:8080/19411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 7
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19411/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19411/3//COMMIT_MSG@9
PS3, Line 9: allow for
           : deleting multiple tablets
nit: allow deletion of multiple tablets


http://gerrit.cloudera.org:8080/#/c/19411/3//COMMIT_MSG@11
PS3, Line 11: operators
nit: users


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

http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/kudu-tool-test.cc@4208
PS3, Line 4208: some specified tablet id patterns do not exist
If tabletid is specified and should work just like a pattern, it would make more sense to say:
"specified tablet id does not exist"


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

http://gerrit.cloudera.org:8080/#/c/19411/3/src/kudu/tools/tool_action_local_replica.cc@1392
PS3, Line 1392: AddOptionalParameter("tables")
I am not sure if I get this. Where are we storing table name from user in case one wants to specify?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 3
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 11 Jan 2023 15:57:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 5: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 5
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 09:38:46 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG@16
PS5, Line 16: can not 
Do you mean "cannot" ?


http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG@16
PS5, Line 16: support
nit: supports


http://gerrit.cloudera.org:8080/#/c/19411/5//COMMIT_MSG@17
PS5, Line 17: whether there is an illegal tablet id pattern well and truly, unless
            : all the specified tablet id patterns are tablet ids.
            : 
Do you mean tablet id patterns will be treated as invalid?


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

http://gerrit.cloudera.org:8080/#/c/19411/5/src/kudu/tools/kudu-tool-test.cc@4320
PS5, Line 4320: --tables=$1
If multiple tables can be entered by user, could you add another test case to cover multiple tables (e.g. 2 tables)?


http://gerrit.cloudera.org:8080/#/c/19411/5/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

http://gerrit.cloudera.org:8080/#/c/19411/5/src/kudu/tools/tool_action_local_replica.cc@793
PS5, Line 793: GetTabletIdsByTableName
nit: It would worth finding out how this performs on scaled configuration with thousands of tablets.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 5
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 16 Jan 2023 15:13:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 7: Code-Review+2

(4 comments)

Overall looks good to me, just a few nits in comments and the commit message.  Thank you for the contribution!

http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG@9
PS7, Line 9: deletion
nit: either 'deleting' or 'deletion of'


http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG@14
PS7, Line 14: delete
nit: deleting


http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG@20
PS7, Line 20: is
was


http://gerrit.cloudera.org:8080/#/c/19411/7/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

http://gerrit.cloudera.org:8080/#/c/19411/7/src/kudu/tools/tool_action_local_replica.cc@234
PS7, Line 234: pattern
nit: patterns



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 7
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Sat, 21 Jan 2023 05:03:43 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow for
deleting multiple tablets by table name. Sometimes it is not
convenient for operators to specify multiple tablet ids, but table
names will be much easier to get.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test was added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 100 insertions(+), 18 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 2
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Ádám Bakai (Code Review)" <ge...@cloudera.org>.
Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19411 )

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 6: Code-Review-1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19411/6/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

http://gerrit.cloudera.org:8080/#/c/19411/6/src/kudu/tools/tool_action_local_replica.cc@801
PS6, Line 801: npos
I think this condition at least needs some rework.I tried:

command: ./bin/kudu local_replica delete 'nonexistent' -fs_data_dirs=$PWD/tserver-1/data  -fs_wal_dir=$PWD/tserver-1/wal  --clean_unsafe --ignore_nonexistent=false
result: Not found: specified tablet id does not exist or does not match table name patterns specified in --tables flag.

command: ./bin/kudu local_replica delete 'nonexistent*' -fs_data_dirs=$PWD/tserver-1/data  -fs_wal_dir=$PWD/tserver-1/wal  --clean_unsafe --ignore_nonexistent=false
result: I20230118 15:49:01.422336 1808955 tool_action_local_replica.cc:809] deleting 0 tablet replicas.

I expected the same behaviour and error message if I give some pattern that maps to no tablets even with or without the "*".



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 6
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Wed, 18 Jan 2023 15:27:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG@9
PS7, Line 9: deleting
> nit: either 'deleting' or 'deletion of'
Done


http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG@14
PS7, Line 14: deleti
> nit: deleting
Done


http://gerrit.cloudera.org:8080/#/c/19411/7//COMMIT_MSG@20
PS7, Line 20: wa
> was
Done


http://gerrit.cloudera.org:8080/#/c/19411/7/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

http://gerrit.cloudera.org:8080/#/c/19411/7/src/kudu/tools/tool_action_local_replica.cc@234
PS7, Line 234: pattern
> nit: patterns
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 8
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>
Gerrit-Comment-Date: Sun, 22 Jan 2023 07:16:14 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] add --tables flag to 'local replica delete'

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow deleting
multiple tablets by table name. Sometimes it is not convenient for
users to specify multiple tablet ids, but table names will be much
easier to get.

In order to support deleting all tablets of specified tables, the
<tablet_ids> argument is updated to <tablet_id_patterns> which
supports basic glob syntax.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag was not specified.

A new test is added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Reviewed-on: http://gerrit.cloudera.org:8080/19411
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 207 insertions(+), 20 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 9
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Ádám Bakai <ab...@cloudera.com>

[kudu-CR] [tools] add --tables flag to 'local replica delete'

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Ashwani Raina, Yingchun Lai, Kudu Jenkins, 

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

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

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

Change subject: [tools] add --tables flag to 'local_replica delete'
......................................................................

[tools] add --tables flag to 'local_replica delete'

This patch updates the 'local_replica delete' tool to allow deletion
multiple tablets by table name. Sometimes it is not convenient for
users to specify multiple tablet ids, but table names will be much
easier to get.

In order to support delete all tablets of specified tables, the
<tablet_ids> argument is updated to <tablet_id_patterns> which
support basic glob syntax. As a result, now we can not tell users
whether there is an illegal tablet id pattern well and truly, unless
all the specified tablet id patterns are tablet ids.

The default value of --tables flag is empty, which means all tables
will be included, so the tool can achieve same effect as before if
this flag is not specified.

A new test was added to verify the new functionality.

Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_local_replica.cc
2 files changed, 102 insertions(+), 17 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d862a715f05179b2e8def0d1cdfe58c32299329
Gerrit-Change-Number: 19411
Gerrit-PatchSet: 4
Gerrit-Owner: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Ashwani Raina <ar...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>