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/08/19 03:07:37 UTC
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18864
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch help to changes default behavior of the DeleteTable RPC at the master side.
If the flag enable, the delete request with 'reserve_seconds' set 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 74 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/1
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18864/12/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/12/src/kudu/master/catalog_manager.cc@411
PS12, Line 411: Reserve seconds
> nit: Time in seconds to be reserved
Done
http://gerrit.cloudera.org:8080/#/c/18864/12/src/kudu/master/catalog_manager.cc@426
PS12, Line 426: would make
> nit: makes
Done
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Sep 2022 06:08:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 17:
(8 comments)
Overall looks good, just a few nits.
I also added a generic comment on how to make the semantics of DeleteTable RPC a bit clearer.
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5221
PS17, Line 5221: ASSERT_FALSE(FLAGS_deleted_table_reserve_seconds);
nit for here and below: please replace with ASSERT_EQ(0, FLAGS_deleted_table_reserve_seconds) for easier mapping into the type of the --deleted_table_reserve_seconds flag
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5222
PS17, Line 5222: = 1;
To prevent flakiness in case of scheduler anomalies and/or slow inferior test VM, maybe set this to something greater than 1 (e.g. 60)?
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5242
PS17, Line 5242: ASSERT_EQ(0, tables.size());
nit: if using ASSERT_TRUE(tables.empty()) at line 5251, maybe use it here as well?
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5258
PS17, Line 5258: ASSERT_FALSE(FLAGS_deleted_table_reserve_seconds);
nit: please replace with ASSERT_EQ(0, FLAGS_deleted_table_reserve_seconds) for easier mapping into the type of the --deleted_table_reserve_seconds flag
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5261
PS17, Line 5261: // Delete the table, the table won't be purged immediately if
: // FLAGS_deleted_table_reserve_seconds set to anything greater than 0.
Is this relevant if non-zero 'reserve_seconds' comes with DeleteTableRequestRPC?
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5279
PS17, Line 5279: ASSERT_EQ(0, tables.size());
nit: if using ASSERT_TRUE(tables.empty()) at line 5288, maybe use it here as well?
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@413
PS17, Line 413:
nit: remove the extra space
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@2419
PS17, Line 2419: // Reserve seconds equal 0 means delete it directly.
: if (req.reserve_seconds() == 0 &&
: (FLAGS_deleted_table_reserve_seconds == 0 || is_soft_deleted_table)) {
: return DeleteTableRpc(req, resp, rpc);
: }
One comment which is a bit off-topic (or just a broader note). It's not necessary to address right in this patch, just curious what you think of this. Having a separate patch to address this is a better option, IMO.
After some consideration, I noticed there is room for improvement on interpreting the 'reserve_seconds' field. That's to make the semantics of DeleteTable clearer and have the behavior to be backward-compatible for older clients that are not aware of the 'reserve_seconds' field in DeleteTableRequestPB.
From what I can see, that requires only cosmetic changes here (and maybe similar cosmetic changes in the client as well).
What if instead of comparing the 'reserve_seconds' with 0, the server side first checked whether the field is set at all in DeleteTableRequestPB (i.e. called DeleteTableRequestPB::has_reserve_seconds())?
If the field is not specified by the client in DeleteTableRequestPB, then we know that's a request either:
* from a legacy client to drop the table immediately
* from a new client to drop the table immediately
In both cases the server side takes the liberty to interpret the request of immediately dropping the table per current setting of its --deleted_table_reserve_seconds flag.
If the field is specified by the client, then we know that's a 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.
With that, the semantics of interpreting DeleteTableRequestPB at the server side becomes a bit clearer, it seems.
For regular a table:
if 'reserve_seconds' field specified in DeleteTableRequestPB then
soft-delete the table with the grace period of 'reserve_seconds' (if that's 0, then the table will be deemed expired and purged as soon as possible by the ExpiredReservedTablesDeleterThread thread)
else
if FLAGS_deleted_table_reserve_seconds != 0 then
soft-delete the table with the grace period of FLAGS_deleted_table_reserve_seconds
else
purge the table immediately
endif
endif
For a soft-deleted table:
if 'reserve_seconds' field specified in DeleteTableRequestPB then
if 'reserve_seconds' == 0 then
immediately purge the table
else
report Status::IllegalState("table is already soft-deleted")
endif
else
report Status::NotFound("table not found")
endif
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 17
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Sep 2022 02:48:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG@9
PS1, Line 9: help to changes
nit: "helps change the"
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG@10
PS1, Line 10: enable
nit: "is enabled"
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG@10
PS1, Line 10: set 0
"set to 0"
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/client/client-test.cc@5210
PS1, Line 5210: No one table
nit: Change to "No tables"
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc@416
PS1, Line 416: DEFINE_uint32(table_reserve_seconds, 60 * 60 * 24 * 7,
Can you include this in the testing of the new flag?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Wed, 24 Aug 2022 23:35:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#4).
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 155 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/4
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 4
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#9).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 123 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/9
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 9
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#2).
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 113 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/2
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 2
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 12:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18864/12/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/12/src/kudu/master/catalog_manager.cc@411
PS12, Line 411: Reserve seconds
nit: Time in seconds to be reserved
http://gerrit.cloudera.org:8080/#/c/18864/12/src/kudu/master/catalog_manager.cc@426
PS12, Line 426: would make
nit: makes
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Sep 2022 02:11:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Sep 2022 17:15:32 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS3:
> Could you please add tests to check how does it act when use the new added
Done
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/client/client-test.cc@5218
PS3, Line 5218: TestReserveSecondsofSoftDeleteEnableOnMaster
> nit: TestReserveSecondsOfSoftDeleteEnableOnMaster
Done
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/client/client-test.cc@5247
PS3, Line 5247: SleepFor(MonoDelta::FromMilliseconds(1000 * 60));
> This will cause the test last too long time, it doesn't obey the rule in ht
Done
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/master/catalog_manager.cc@413
PS3, Line 413: "NOTE : it's useless if HMS is enable."
> How about use GROUP_FLAG_VALIDATOR to check it?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 29 Aug 2022 03:49:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#18).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 157 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/18
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 18
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc@409
PS1, Line 409: enable_soft_delete_on_master
> I am trying understand more on this flag. So by default this is disabled bu
Usually, the client and server need to be updated at the same time for a new functions. This flag is added to enable soft deletion without updating the client.
So you got my intention that we support soft-delete even this flag is diaable and reserved_seconds is not 0 in the request.
By the way, this is the current situation without considering this new flag.
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 24 Aug 2022 06:16:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 20: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@2419
PS17, Line 2419:
: // Reserve seconds equal 0 means delete it directly if default_deleted_table_reserve_seconds
: // set to 0, which means the cluster-wide behavior of the DeleteTable() RPC is keep default,
: // or the table is in soft-deleted status.
: i
> Yes, you put forward a very good improvement point. I am very happy to prop
I'm glad you liked the idea :)
Yes, it would be great if you could post a patch to implement that as above. Thank you very much for your contributions!
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 20
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Sep 2022 17:30:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#17).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 158 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/17
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 17
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#12).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 123 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/12
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#7).
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 155 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/7
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 20: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 20
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Sep 2022 07:04:48 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#5).
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 155 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/5
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 5
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc@409
PS1, Line 409: enable_soft_delete_on_master
I am trying understand more on this flag. So by default this is disabled but we still support soft deletes if reserve_seconds is not 0 in the request?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 24 Aug 2022 04:18:26 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#8).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 122 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/8
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 8
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 17: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 17
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 12 Sep 2022 16:51:18 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc@409
PS1, Line 409: enable_soft_delete_on_master
> But shouldn't the ideal behavior be - if the soft deletes are disabled on t
Yes, I think your idea is applicable to the enabling of most new functions. But the soft-delete function is designed to increainge the fault tolerance of the Kudu system. This is same to enable the soft-delete on the master side to deal with the delete request without soft-delete set.
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 01 Sep 2022 06:22:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#11).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 123 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/11
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 11
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/15/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/15/src/kudu/master/catalog_manager.cc@2420
PS15, Line 2420: if (req.reserve_seconds() == 0) {
: if (FLAGS_deleted_table_reserve_seconds == 0 || is_soft_deleted_table) {
nit: Can we merge these two if conditions?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 15
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Sep 2022 21:53:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE: this flag is useless if HMS is enabled.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Reviewed-on: http://gerrit.cloudera.org:8080/18864
Tested-by: Kudu Jenkins
Reviewed-by: Yingchun Lai <ac...@gmail.com>
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 162 insertions(+), 5 deletions(-)
Approvals:
Kudu Jenkins: Verified
Yingchun Lai: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 21
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#3).
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 113 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/3
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG@9
PS1, Line 9: help to changes
> nit: "helps change the"
Done
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG@10
PS1, Line 10: enable
> nit: "is enabled"
Done
http://gerrit.cloudera.org:8080/#/c/18864/1//COMMIT_MSG@10
PS1, Line 10: set 0
> "set to 0"
Done
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/client/client-test.cc@5210
PS1, Line 5210: No one table
> nit: Change to "No tables"
Done
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc@416
PS1, Line 416: DEFINE_uint32(table_reserve_seconds, 60 * 60 * 24 * 7,
> Can you include this in the testing of the new flag?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Aug 2022 02:46:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#16).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 158 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/16
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 16
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 17:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5221
PS17, Line 5221: ASSERT_FALSE(FLAGS_deleted_table_reserve_seconds);
> nit for here and below: please replace with ASSERT_EQ(0, FLAGS_deleted_tabl
Done
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5222
PS17, Line 5222: = 1;
> To prevent flakiness in case of scheduler anomalies and/or slow inferior te
I used to set this 60, but Yingchun Lai suggest to obey the rule in https://kudu.apache.org/docs/contributing.html#_testing.
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5242
PS17, Line 5242: ASSERT_EQ(0, tables.size());
> nit: if using ASSERT_TRUE(tables.empty()) at line 5251, maybe use it here a
Done
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5258
PS17, Line 5258: ASSERT_FALSE(FLAGS_deleted_table_reserve_seconds);
> nit: please replace with ASSERT_EQ(0, FLAGS_deleted_table_reserve_seconds)
Done
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5261
PS17, Line 5261: // Delete the table, the table won't be purged immediately if
: // FLAGS_deleted_table_reserve_seconds set to anything greater than 0.
> Is this relevant if non-zero 'reserve_seconds' comes with DeleteTableReques
Removed.
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5279
PS17, Line 5279: ASSERT_EQ(0, tables.size());
> nit: if using ASSERT_TRUE(tables.empty()) at line 5288, maybe use it here a
Done
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@413
PS17, Line 413:
> nit: remove the extra space
Done
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@2419
PS17, Line 2419: // Reserve seconds equal 0 means delete it directly.
: if (req.reserve_seconds() == 0 &&
: (FLAGS_deleted_table_reserve_seconds == 0 || is_soft_deleted_table)) {
: return DeleteTableRpc(req, resp, rpc);
: }
> One comment which is a bit off-topic (or just a broader note). It's not ne
Yes, you put forward a very good improvement point. I am very happy to propose a new patch to realize your idea.
Thank you very much for your advice!
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 17
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 15 Sep 2022 07:46:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 12:
(2 comments)
Thanks for your reviews.
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG@9
PS7, Line 9:
: This patch helps to change the default behavior of the DeleteTable RPC at the master side.
: Value 0 means DeleteTable() works the regular way, i.e. dro
> Could we make it a bit simpler?
Done
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG@9
PS7, Line 9:
: This patch helps to change the default behavior of the DeleteTable RPC at the master side.
: Value 0 means DeleteTable() works the regular way, i.e. dro
> +1
Done
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 12
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Tue, 06 Sep 2022 02:08:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2422
PS14, Line 2422: req
Isn't it necessary to update the 'reserve_seconds' field in 'req' when calling SoftDeleteTable()?
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2421
PS14, Line 2421: if (FLAGS_deleted_table_reserve_seconds > 0) {
: return SoftDeleteTable(req, resp, rpc);
: }
What if the table is already soft-deleted at this point? The code at lines 2409-2417 checks for the soft-deleted status based on req.reserve_seconds(), but now SoftDeleteTable() is called regardless.
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Sep 2022 02:40:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2422
PS14, Line 2422: req
> Isn't it necessary to update the 'reserve_seconds' field in 'req' when call
Yes, the update op is necessary and I do this at line 2469.
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2421
PS14, Line 2421: if (FLAGS_deleted_table_reserve_seconds > 0) {
: return SoftDeleteTable(req, resp, rpc);
: }
> What if the table is already soft-deleted at this point? The code at lines
The GetTableStates(...) function checks the soft-deleted status base the metadata state, not the req.reserver_seconds(). So it's impossible we get a soft-deleted status table at this point.
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Sep 2022 03:21:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2422
PS14, Line 2422: req
> Yes, the update op is necessary and I do this at line 2469.
Ah, indeed. That's a bit strange that the same condition on FLAGS_deleted_table_reserve_seconds is involved twice, though.
Also, is it possible to update this section of the code to have just one call site for SoftDeleteTable(), not two? As of PS14, SoftDeleteTable() has two call sites in this small code section: lines 2422 and 2429. I hope that having just one call site could make the code more readable.
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2421
PS14, Line 2421: if (FLAGS_deleted_table_reserve_seconds > 0) {
: return SoftDeleteTable(req, resp, rpc);
: }
> The GetTableStates(...) function checks the soft-deleted status base the me
I'm not sure I understand that explanation.
From the code I can see that nothing prevents sending DeleteTableRequestPB() for a soft-deleted, but not yet purged table, right? Now, what if FLAGS_deleted_table_reserve_seconds > 0 for the cluster? From the code I can see that SoftDeleteTable() would be called again for the soft-deleted table.
If you insist that's impossible, could you then please add a test scenario for the following:
1) A Kudu cluster runs with --deleted_table_reserve_seconds=600
2) A table X is created
3) A request to soft-delete table X arrives with reserve_seconds == 60
4) Right after that, a request to purge the soft-deleted table X arrives
5) The result should be: the table X should be immediately purged, so no table X should be in the system after that.
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Sep 2022 04:40:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#15).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 159 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/15
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 15
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS3:
Could you please add tests to check how does it act when use the new added client APIs (i.e. SoftDeleteTable, RecallTable) to operate on the cluster when FLAGS_enable_soft_delete_on_master is false?
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/client/client-test.cc@5218
PS3, Line 5218: TestReserveSecondsofSoftDeleteEnableOnMaster
nit: TestReserveSecondsOfSoftDeleteEnableOnMaster
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/client/client-test.cc@5247
PS3, Line 5247: SleepFor(MonoDelta::FromMilliseconds(1000 * 60));
This will cause the test last too long time, it doesn't obey the rule in https://kudu.apache.org/docs/contributing.html#_testing. Could you reduce the reserve time, maybe the soft deleted table check interval on master should also to be reduced.
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/3/src/kudu/master/catalog_manager.cc@413
PS3, Line 413: "NOTE : it's useless if HMS is enable."
How about use GROUP_FLAG_VALIDATOR to check it?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Aug 2022 09:16:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG@9
PS7, Line 9: This patch help to changes default behavior of the DeleteTable RPC at the master side.
: If the flag enable, the delete request with 'reserve_seconds' set 0 would make no sense,
: the table will do soft-delete instead of purge immediately.
> Could we make it a bit simpler?
+1
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 01 Sep 2022 20:22:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#20).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE: this flag is useless if HMS is enabled.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 162 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/20
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 20
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 20:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/18864/19//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/19//COMMIT_MSG@15
PS19, Line 15: NOTE: this flag is useless if HMS is enabled.
> nit: how about:
Done
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5222
PS17, Line 5222:
> I agree 1 second maybe too short, maybe 3 or 5 seconds? And besides, you ca
Done
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@411
PS19, Line 411:
> nit: alignment
Done
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@416
PS19, Line 416: n
> I guess that's because the reserve seconds has been specified by user while
Done
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@425
PS19, Line 425: e
> nit: If
Done
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@2419
PS19, Line 2419:
> nit: update the comments?
Done
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@2466
PS19, Line 2466: elete state
> nit: Still naming 'reserve_seconds'?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 20
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 19 Sep 2022 03:44:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#13).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 123 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/13
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 13
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#14).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 123 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/14
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#10).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 123 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/10
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 10
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "Abhishek Chennaka (Code Review)" <ge...@cloudera.org>.
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/1/src/kudu/master/catalog_manager.cc@409
PS1, Line 409: enable_soft_delete_on_master
> Usually, the client and server need to be updated at the same time for a ne
But shouldn't the ideal behavior be - if the soft deletes are disabled on the master and the client tries to do a soft delete, it should receive an error message saying it is disabled on the server end?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 01 Sep 2022 04:27:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/7//COMMIT_MSG@9
PS7, Line 9: This patch helps to change the default behavior of the DeleteTable RPC at the master side.
: If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
: the table will do soft-delete instead of purge immediately.
Could we make it a bit simpler?
I guess the idea is to have a flag to change the cluster-wide behavior of the DeleteTable() RPC. That's to make all DeleteTable requests to actually turn into SoftDeletTable() with the specified grace period at the server side.
The use case: there is a bunch of already deployed Kudu applications that call DeleteTable since they are not aware of SoftDeleteTable and it's not possible to modify those applications, but it's necessary to treat each delete table request with some grace period, so it's possible to recall dropped tables for a limited time after DeleteTable request arrives. And it's necessary to do so cluster-wide.
As for the explicit calls of the SoftDeleteTable() RPC, I think it should always be treated as-is with the exact grace/reserve period for the dropped tables. If such RPC calls arrives, that means the application is aware of SoftDeleteTable() RPC, so it wants to have the table soft-deleted with the exact specified grace/reserve time.
With that, I think it's enough to introduce just a single flag --deleted_table_reserve_seconds, with the default value of 0. That's the flag to control the cluster-wide behavior of the DeleteTable() RPC: if the flag is set to 0, then DeleteTable() works the regular/legacy way, i.e. dropping the table and purging its data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds).
I don't think we need to introduce a public kill-switch flag like --enable_soft_delete_on_master unless we want a way to disable soft-delete table support in newer servers. We might add that just for testing purposes, though. What we actually need to do is to introduce a new feature (TABLE_SOFT_DELETION) for masters, and the clients should required that feature when sending DeleteTable() RPCs with non-zero 'reserve_seconds' field. That way the client will know that an old server not just ignore the 'reserve_seconds' field it doesn't recognize, but instead will get a proper error response to realize that the server doesn't support the soft-delete functionality for tables.
What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 01 Sep 2022 19:17:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 14
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Wed, 07 Sep 2022 19:32:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Alexey Serbin, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#19).
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide
behavior of the DeleteTable() RPC.
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
Value 0 means DeleteTable() works the regular way, i.e. dropping the table and purging its
data immediately. If it's set to anything greater than 0, then all DeleteTable() RPCs are
turned into SoftDeleteTable(..., FLAGS_deleted_table_reserve_seconds)").
NOTE : this flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 157 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/19
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 19
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18864/15/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/15/src/kudu/master/catalog_manager.cc@2420
PS15, Line 2420: if (req.reserve_seconds() == 0 &&
: (FLAGS_deleted_table_reserve_seconds == 0 || is_soft_deleted_table)) {
> nit: Can we merge these two if conditions?
Done
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 16
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Sep 2022 01:59:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2422
PS14, Line 2422: eq,
> Ah, indeed. That's a bit strange that the same condition on FLAGS_deleted_
Done
http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2421
PS14, Line 2421: if (FLAGS_deleted_table_reserve_seconds == 0 || is_soft_deleted_table) {
: return DeleteTableRpc(req, resp, rpc);
: }
> I'm not sure I understand that explanation.
Yes, I added the test case you mentioned and found that I need to modify the code. Could you help ti check this again? Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 15
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Thu, 08 Sep 2022 06:40:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3326 [master] add flag FLAGS enable soft delete on master to enable soft-delete on master
Posted by "KeDeng (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Yingchun Lai, Kudu Jenkins, Abhishek Chennaka,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18864
to look at the new patch set (#6).
Change subject: KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
......................................................................
KUDU-3326 [master] add flag FLAGS_enable_soft_delete_on_master to enable soft-delete on master
This patch helps to change the default behavior of the DeleteTable RPC at the master side.
If the flag is enabled, the delete request with 'reserve_seconds' set to 0 would make no sense,
the table will do soft-delete instead of purge immediately.
NOTE : the flag is useless if HMS is enable.
Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
2 files changed, 155 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/18864/6
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 6
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
[kudu-CR] KUDU-3326 [master] add flag FLAGS deleted table reserve seconds to change the cluster-wide behavior of the DeleteTable() RPC.
Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC.
......................................................................
Patch Set 19:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/18864/19//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18864/19//COMMIT_MSG@15
PS19, Line 15: NOTE : this flag is useless if HMS is enable.
nit: how about:
NOTE: this flag is useless if HMS is enabled.
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5222
PS17, Line 5222:
> I used to set this 60, but Yingchun Lai suggest to obey the rule in https:/
I agree 1 second maybe too short, maybe 3 or 5 seconds? And besides, you can use SKIP_IF_SLOW_NOT_ALLOWED() to skip this test.
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@411
PS19, Line 411: "
nit: alignment
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@416
PS19, Line 416: .
I guess that's because the reserve seconds has been specified by user while calling SoftDeleteTable? Add some comments to explain that?
And I suggest to add 'default' word in this flag.
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@425
PS19, Line 425: i
nit: If
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@2419
PS19, Line 2419: // Reserve seconds equal 0 means delete it directly.
nit: update the comments?
http://gerrit.cloudera.org:8080/#/c/18864/19/src/kudu/master/catalog_manager.cc@2466
PS19, Line 2466: reserve_time
nit: Still naming 'reserve_seconds'?
--
To view, visit http://gerrit.cloudera.org:8080/18864
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc
Gerrit-Change-Number: 18864
Gerrit-PatchSet: 19
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Abhishek Chennaka <ac...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Mon, 19 Sep 2022 02:53:09 +0000
Gerrit-HasComments: Yes