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 <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Tue, 06 Oct 2020 04:11:52 +0000 Gerrit-HasComments: Yes
