You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2020/10/04 02:21:36 UTC
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16544
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
[catalog_manager] Status::AlreadyPresent for range duplicates
With this patch, CatalogManager discriminates between overlapped
and exact duplicate key ranges when adding new partitions, returning
Status::AlreadyPresent() for exact range duplicates and
Status::InvalidArgument for otherwise overlapped ones. The number
of hash buckets is not taken into account when searching/reporting
about the exact duplicates in key range for partitions.
Before this patch, CatalogManager returned Status::InvalidArgument()
for exact duplicates and otherwise overlapped ranges as well.
This patch also updates the relevant tests, so the new behavior
has enough test coverage.
A follow-up patch will use the newly introduced finer status reporting.
To be more precise, TxnManager could send multiple requests to add the
same new tablet for the transaction status table while processing
concurrent BeginTransaction() requests. With that, it should be able
to tell between an error when the corresponding tablet is already
present and other types of errors.
Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
---
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/catalog_manager.cc
3 files changed, 128 insertions(+), 90 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16544/1
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@16
PS2, Line 16: Before this patch, CatalogManager returned Status::InvalidArgument()
: for exact duplicates and otherwise overlapped ranges as well.
Seems client will expect to see different error status being returned before and after the change. Wondering do you foresee will this raise any compatibility concerns?
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Oct 2020 22:52:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@16
PS2, Line 16: Before this patch, CatalogManager returned Status::InvalidArgument()
: for exact duplicates and otherwise overlapped ranges as well.
> Right -- in case of exact range match the client will now get Status::Alrea
Ok, sounds good to me as long as we document it in the release notes.
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Oct 2020 18:17:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 2:
Tagging Mahesh just for more exposure to more range partitioning code.
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Oct 2020 22:07:10 +0000
Gerrit-HasComments: No
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@16
PS2, Line 16: Before this patch, CatalogManager returned Status::InvalidArgument()
: for exact duplicates and otherwise overlapped ranges as well.
> Seems client will expect to see different error status being returned befor
Right -- in case of exact range match the client will now get Status::AlreadyExists() instead of Status::InvaligArgument(). As I can see from the client.h, the exact meaning of the return codes hasn't been documented, but I guess it will be necessary to document this change in the release notes.
I don't see this as a detrimental change -- instead, this patch improves the interface for this particular use case since Status::InvalidArgument() returned for other things like missing alter specification for table and column schema, invalid specification of key columns, etc.
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@25
PS2, Line 25: concurrent BeginTransaction() requests. With that, it should be able
> Sure, I guess I'm trying to piece together how this will be used by the Txn
Right -- the idea is to make sure that's TxnManager who adds the partitions, and that will work even if TxnManagers request not equally-sized partitions, but just partitions of the same size to accommodate particular transaction identifier. That logic is already present in WIP patch: https://gerrit.cloudera.org/#/c/16527/1/src/kudu/master/txn_manager.cc@154
I revved that patch and added a test to calls TxnManager::BeginTransaction() concurrently, and found I need finer status reporting because Status::InvalidArgument() is too broad for this use case given the number of places in the AlterTable RPC's control flow which could result in Status::InvalidArgument(). I think TxnManager() and other use cases will benefit from this clear-cut semantics when concurrent actors request to create the same range partition.
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc@1420
PS2, Line 1420: // ADD [0, 100) <- already present (duplicate)
> What about the case where the upper or lower bounds don't exist (i.e. we ha
See lines 2009 and 2041 of PS2 for the coverage of such cases.
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc@1428
PS2, Line 1428: IsAlreadyPresent
> Looking around the client docs a bit, I don't think we document the specifi
Yup. In addition, this change makes detecting the condition of already existing tablet with exact ranges much cleaner.
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc@2467
PS2, Line 2467: tablet directly *after*
> nit: could you clarify this comment to indicate which bounds of the existin
Done
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Oct 2020 04:11:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
[catalog_manager] Status::AlreadyPresent for range duplicates
With this patch, CatalogManager discriminates between overlapped
and exact duplicate key ranges when adding new partitions, returning
Status::AlreadyPresent() for exact range duplicates and
Status::InvalidArgument for otherwise overlapped ones. The number
of hash buckets is not taken into account when searching/reporting
about the exact duplicates in key range for partitions.
Before this patch, CatalogManager returned Status::InvalidArgument()
for exact duplicates and otherwise overlapped ranges as well.
This patch also updates the relevant tests, so the new behavior
has enough test coverage.
A follow-up patch will use the newly introduced finer status reporting.
To be more precise, TxnManager could send multiple requests to add the
same new tablet for the transaction status table while processing
concurrent BeginTransaction() requests. With that, it should be able
to tell between an error when the corresponding tablet is already
present and other types of errors.
Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Reviewed-on: http://gerrit.cloudera.org:8080/16544
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/catalog_manager.cc
4 files changed, 148 insertions(+), 102 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@25
PS2, Line 25: concurrent BeginTransaction() requests. With that, it should be able
> At least from the point of pure semantics, returning AlreadyPresent for dup
Sure, I guess I'm trying to piece together how this will be used by the TxnManager. I'm assuming something like: if we make a request and get back AlreadyPresent, a previous request (from this TxnManager or another one) has succeeded and we can proceed with the next txn ID. We never expect to get back InvalidArgument because we will assume that all TxnManagers will request equally-sized range blocks. Is that all correct?
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc@1420
PS2, Line 1420: // ADD [0, 100) <- already present (duplicate)
What about the case where the upper or lower bounds don't exist (i.e. we have unbounded partitions)?
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc@1428
PS2, Line 1428: IsAlreadyPresent
Looking around the client docs a bit, I don't think we document the specific error code, and so I suppose it's OK to change this error code.
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc@2467
PS2, Line 2467: tablet directly *after*
nit: could you clarify this comment to indicate which bounds of the existing_tablets its comparing against? E.g. is this comparing against the lower bounds of existing tablets? Also what does it mean for iter to equal existing_tablets.begin()?
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Oct 2020 23:29:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@25
PS2, Line 25: concurrent BeginTransaction() requests. With that, it should be able
> Curious why we wouldn't have been able to just change all of these to Alrea
At least from the point of pure semantics, returning AlreadyPresent for duplicate ranges is much cleaner, IMO. From that perspective, InvalidArgument is some sort of a broader status code, but this is not safe to assume otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Mon, 05 Oct 2020 22:29:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 3:
Thank you for the review!
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Oct 2020 17:59:50 +0000
Gerrit-HasComments: No
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 3: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@25
PS2, Line 25: concurrent BeginTransaction() requests. With that, it should be able
> Right -- the idea is to make sure that's TxnManager who adds the partitions
Ack
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc@1420
PS2, Line 1420: // ADD [0, 100) <- already present (duplicate)
> See lines 2009 and 2041 of PS2 for the coverage of such cases.
Ah, thanks for the pointer!
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc@2467
PS2, Line 2467: elements of 'existing_ta
> Done
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Oct 2020 17:51:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 )
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@25
PS2, Line 25: concurrent BeginTransaction() requests. With that, it should be able
Curious why we wouldn't have been able to just change all of these to AlreadyPresent? I.e. why do we need to know about an exact match, vs knowing about range overlap in general?
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 05 Oct 2020 22:06:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16544
to look at the new patch set (#2).
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
[catalog_manager] Status::AlreadyPresent for range duplicates
With this patch, CatalogManager discriminates between overlapped
and exact duplicate key ranges when adding new partitions, returning
Status::AlreadyPresent() for exact range duplicates and
Status::InvalidArgument for otherwise overlapped ones. The number
of hash buckets is not taken into account when searching/reporting
about the exact duplicates in key range for partitions.
Before this patch, CatalogManager returned Status::InvalidArgument()
for exact duplicates and otherwise overlapped ranges as well.
This patch also updates the relevant tests, so the new behavior
has enough test coverage.
A follow-up patch will use the newly introduced finer status reporting.
To be more precise, TxnManager could send multiple requests to add the
same new tablet for the transaction status table while processing
concurrent BeginTransaction() requests. With that, it should be able
to tell between an error when the corresponding tablet is already
present and other types of errors.
Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/catalog_manager.cc
4 files changed, 137 insertions(+), 99 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16544/2
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Mahesh Reddy, Attila Bukor, Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16544
to look at the new patch set (#3).
Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates
......................................................................
[catalog_manager] Status::AlreadyPresent for range duplicates
With this patch, CatalogManager discriminates between overlapped
and exact duplicate key ranges when adding new partitions, returning
Status::AlreadyPresent() for exact range duplicates and
Status::InvalidArgument for otherwise overlapped ones. The number
of hash buckets is not taken into account when searching/reporting
about the exact duplicates in key range for partitions.
Before this patch, CatalogManager returned Status::InvalidArgument()
for exact duplicates and otherwise overlapped ranges as well.
This patch also updates the relevant tests, so the new behavior
has enough test coverage.
A follow-up patch will use the newly introduced finer status reporting.
To be more precise, TxnManager could send multiple requests to add the
same new tablet for the transaction status table while processing
concurrent BeginTransaction() requests. With that, it should be able
to tell between an error when the corresponding tablet is already
present and other types of errors.
Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
---
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/txn_status_table-itest.cc
M src/kudu/master/catalog_manager.cc
4 files changed, 148 insertions(+), 102 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/16544/3
--
To view, visit http://gerrit.cloudera.org:8080/16544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489
Gerrit-Change-Number: 16544
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>