You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "KeDeng (Code Review)" <ge...@cloudera.org> on 2022/09/27 11:55:00 UTC
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19047
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
10 files changed, 88 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/1
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#12).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 106 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/12
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (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/19047
to look at the new patch set (#7).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 100 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/7
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 6:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/19047/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/19047/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@719
PS5, Line 719: '--default_deleted_table_reserve_seconds' fla
> nit: '--default_deleted_table_reserve_seconds' flag on master.
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@687
PS5, Line 687: /// The deleted table may turn to soft-deleted status with the flag
> nit: --default_deleted_table_reserve_seconds
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747
PS5, Line 747:
> How about set reserve_seconds as std::optional<uint32_t>, then with_reserve
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2413
PS5, Line 2413: The only error is NotFound
> How about adjust the code like:
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2418
PS5, Line 2418: DCHECK(s.ok());
> nit: --default_deleted_table_reserve_seconds
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2421
PS5, Line 2421: // Kudu client with the precise value for 'reserve_seconds', and the field's value from the
> nit: --default_deleted_table_reserve_seconds
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2422
PS5, Line 2422: est should be taken as-is re
> How about
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2423
PS5, Line 2423: seconds' flag at th
> reserve_seconds
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2428
PS5, Line 2428: bool enable_soft_delete_on_master = FLAGS_default_deleted_table_reserve_seconds != 0;
> It's not needed to check if 's' is OK or not, has been checked before in li
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433
PS5, Line 2433: // soft-delete has enabled
> nit: 'soft deleted' or 'soft-deleted'
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433
PS5, Line 2433:
> nit: 'soft deleted again' ?
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/tools/tool_action_table.cc@201
PS5, Line 201: DEFINE_int32(reserve_seconds, -1,
> I guess it's not necessary if set the --reserve_seconds default value as -1
Done
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 6
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 31 Oct 2022 03:13:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 14:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19047/12/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java:
http://gerrit.cloudera.org:8080/#/c/19047/12/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java@37
PS12, Line 37: private int reserveSeconds = -1;
> The same to C++ client, is the Boolean variable really needed?
Done
http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h@740
PS13, Line 740: nds a
> nit: field?
removed
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747
PS5, Line 747:
> What's the failure details?
Done
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@202
PS12, Line 202: reserve_seconds
> I‘d prefer the original 'reserve_seconds', it’s corresponding to the server
Done
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 07 Nov 2022 03:44:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 5:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/19047/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/19047/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@719
PS5, Line 719: 'default_deleted_table_reserve_seconds' flag.
nit: '--default_deleted_table_reserve_seconds' flag on master.
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@687
PS5, Line 687: /// default_deleted_table_reserve_seconds set to nonzero on the master side.
nit: --default_deleted_table_reserve_seconds
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747
PS5, Line 747: uint32_t reserve_seconds = 0) KUDU_NO_EXPORT;
How about set reserve_seconds as std::optional<uint32_t>, then with_reserve_seconds is not needed.
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2413
PS5, Line 2413: The only error is NotFound
How about adjust the code like:
// The only error is NotFound, ...
if (s.IsNotFound()) {
return DeleteTableRpc(req, resp, rpc);
}
DCHECK(s.ok());
...
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2418
PS5, Line 2418: // 'default_deleted_table_reserve_seconds' flag at the server side.
nit: --default_deleted_table_reserve_seconds
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2421
PS5, Line 2421: // controlled by the 'default_deleted_table_reserve_seconds' flag.
nit: --default_deleted_table_reserve_seconds
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2422
PS5, Line 2422: enable_soft_delete_on_client
How about
bool enable_soft_delete_on_client = req.has_reserve_seconds() && req.reserve_seconds() != 0;
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2423
PS5, Line 2423: has_reserve_seconds
reserve_seconds
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2428
PS5, Line 2428: if (s.ok() && is_soft_deleted_table &&
It's not needed to check if 's' is OK or not, has been checked before in line 2414.
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433
PS5, Line 2433: deleted
nit: 'soft deleted again' ?
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433
PS5, Line 2433: Status::InvalidArgument(Substitute("soft_deleted table $0 should not be deleted",
nit: 'soft deleted' or 'soft-deleted'
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/tools/tool_action_table.cc@201
PS5, Line 201: DEFINE_bool(with_reserve_seconds, false,
I guess it's not necessary if set the --reserve_seconds default value as -1. Then 0 means not reserve, and larger than 0 means reserve with some seconds?
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 5
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sun, 30 Oct 2022 11:42:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#2).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 108 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/2
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (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/19047
to look at the new patch set (#5).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 119 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/5
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 5
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#13).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 110 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/13
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 13
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 14: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h@740
PS13, Line 740: nds a
> removed
Done
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Nov 2022 02:46:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2412
PS3, Line 2412: Status s = GetTableStates(req.table(), kAllTableType, &is_soft_deleted_table, &is_expired_table);
The error status should be dealt at first? Now the only error is NotFound.
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2414
PS3, Line 2414: the
nit: remove it
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2418
PS3, Line 2418: if (req.has_reserve_seconds()) {
Maybe these two cases (whether use specified 'reserve_seconds' or not) can be unified to simlify code, since there are almost duplicate code?
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto@591
PS3, Line 591: 'default_deleted_table_reserve_seconds'
nit: it's a habit to add two minus chars before the flag, i.e. '--default_deleted_table_reserve_seconds', to expose that it's a flag.
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto@593
PS3, Line 593: default_deleted_table_reserve_seconds
nit: same
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/tools/tool_action_table.cc@201
PS3, Line 201: DEFINE_bool(with_reserve_seconds, false,
If '--reserve_seconds' is not 0, that implicit says that it is specified and we should use it, so is it duplicate to add such a '--with_reserve_seconds' flag?
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sat, 15 Oct 2022 13:29:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#9).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/integration-tests/master_hms-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
14 files changed, 103 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/9
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 9
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#11).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 106 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/11
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 11
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#3).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 118 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/3
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2412
PS3, Line 2412: Status s = GetTableStates(req.table(), kAllTableType, &is_soft_deleted_table, &is_expired_table);
> The error status should be dealt at first? Now the only error is NotFound.
Done
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2414
PS3, Line 2414: the
> nit: remove it
Done
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2418
PS3, Line 2418: if (req.has_reserve_seconds()) {
> Maybe these two cases (whether use specified 'reserve_seconds' or not) can
Done
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto@591
PS3, Line 591: 'default_deleted_table_reserve_seconds'
> nit: it's a habit to add two minus chars before the flag, i.e. '--default_d
Done
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto@593
PS3, Line 593: default_deleted_table_reserve_seconds
> nit: same
Done
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/tools/tool_action_table.cc@201
PS3, Line 201: DEFINE_bool(with_reserve_seconds, false,
> If '--reserve_seconds' is not 0, that implicit says that it is specified an
If '--reserve_seconds' is 0, it's hard for us to distinguish whether it is the default value. This flag is used to ensure uniform behavior of different versions and make the '--reserve_seconds' more significant.
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 18 Oct 2022 09:13:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#8).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 100 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/8
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 8
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 13:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/19047/12/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
File java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java:
http://gerrit.cloudera.org:8080/#/c/19047/12/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java@37
PS12, Line 37: private Boolean withReserveSeconds = false;
The same to C++ client, is the Boolean variable really needed?
http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h@740
PS13, Line 740: filed
nit: field?
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747
PS5, Line 747: uint32_t reserve_seconds = 0) KUDU_NO_EXPORT;
> I'm sorry I failed to change this because this will make python UT fail.
What's the failure details?
Or we can use flag --soft_table_reserve_time_s in tool_action_table.cc, that is -1 means not specified(use server side config), 0 means purge immediately, larger than 0 means reserve some seconds.
After all, it would be better to reduce the count pf parameters, and avoid confict like, set with_reserve_seconds=false but set reserve_seconds=120.
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@202
PS12, Line 202: soft_table_rese
> Done
I‘d prefer the original 'reserve_seconds', it’s corresponding to the server side flag 'default_deleted_table_reserve_seconds', here the flag is only used in 'kudu table delete' command, so something like 'deleted, table' can be omitted. "soft table" is a bit of strange...
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 13
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sun, 06 Nov 2022 13:33:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 13:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/19047/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19047/12//COMMIT_MSG@10
PS12, Line 10: a new
> nit: a
Done
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client-test.cc@5358
PS12, Line 5358: The
> nit: The
Done
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747
PS5, Line 747: uint32_t reserve_seconds = 0) KUDU_NO_EXPORT;
> Done
I'm sorry I failed to change this because this will make python UT fail.
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client.cc@513
PS12, Line 513: param 'reserve
> The api doesn't exist 'reserve_seconds', should comments be changed?
It's a default param, I will make it clear.
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/catalog_manager.cc@2414
PS12, Line 2414: if (s.IsNotFound()) {
: return DeleteTableRpc(req, resp, rpc);
: }
> Table is not found, execute DeleteTableRpc for the table? Maybe it's triv
Beacuse we do not authorsize the request. The result should not be exposed to unauthenticated users.
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/catalog_manager.cc@2429
PS12, Line 2429: == 0
> How about req.reserve_seconds() < 0?
The reserve_seconds is uint32 type, so the judgement is enough.
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/master.proto@589
PS12, Line 589: a new
> nit: a
Done
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@202
PS12, Line 202: soft_table_rese
> reserve_time_s, soft_table_reserve_time_s ?
Done
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@204
PS12, Line 204: it would be purged when a table is drop
> It would be purged when a table is dropped/deleted.
Done
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 13
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Nov 2022 09:17:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (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/19047
to look at the new patch set (#4).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 112 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/4
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 4
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#10).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 106 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/10
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 10
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (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/19047
to look at the new patch set (#6).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 100 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/6
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 6
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Yuqi Du, Yingchun Lai, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/19047
to look at the new patch set (#14).
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 98 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/47/19047/14
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
If the field 'reserve_seconds' is specified by the client, then it's clear the delete request
coming from a newer Kudu client with the precise value for 'reserve_seconds', and the
field's value from the request should be taken as-is regardless of the current setting of the
--deleted_table_reserve_seconds flag at the server side.
Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Reviewed-on: http://gerrit.cloudera.org:8080/19047
Tested-by: Kudu Jenkins
Reviewed-by: Yingchun Lai <ac...@gmail.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_table.cc
13 files changed, 98 insertions(+), 39 deletions(-)
Approvals:
Kudu Jenkins: Verified
Yingchun Lai: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 15
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC
Posted by "Yuqi Du (Code Review)" <ge...@cloudera.org>.
Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC
......................................................................
Patch Set 12:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/19047/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/19047/12//COMMIT_MSG@10
PS12, Line 10: the a
nit: a
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client-test.cc@5358
PS12, Line 5358: the
nit: The
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/client/client.cc@513
PS12, Line 513: reserve_seconds
The api doesn't exist 'reserve_seconds', should comments be changed?
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/catalog_manager.cc@2414
PS12, Line 2414: if (s.IsNotFound()) {
: return DeleteTableRpc(req, resp, rpc);
: }
Table is not found, execute DeleteTableRpc for the table? Maybe it's triviality, but a little strange.
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/catalog_manager.cc@2429
PS12, Line 2429: == 0
How about req.reserve_seconds() < 0?
And two line codes above about ' < 0' .
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/master/master.proto@589
PS12, Line 589: the a
nit: a
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@202
PS12, Line 202: reserve_seconds
reserve_time_s, soft_table_reserve_time_s ?
http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@204
PS12, Line 204: table is purged once it dropped/deleted
It would be purged when a table is dropped/deleted.
--
To view, visit http://gerrit.cloudera.org:8080/19047
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f
Gerrit-Change-Number: 19047
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Thu, 03 Nov 2022 08:22:34 +0000
Gerrit-HasComments: Yes