Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22420 )
Change subject: KUDU-3639 Add REST-compatible table operation functions ...................................................................... Patch Set 8: (14 comments) Thank you for working on this! I wasn't in the list of reviewers, but I glanced over the updated code in my git workpace after pulling in the recent updates, so I decided to chime in even if this patch is already pushed. http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc File src/kudu/master/catalog_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@297 PS8, Line 297: KUDU_CHECK_OK( This should have been RETURN_NOT_OK instead. http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@301 PS8, Line 301: KuduTableCreator* tableCreator Wrap this into std::unique_ptr and remove 'delete' at line 312. It would be much less error prone that way. http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@307 PS8, Line 307: KUDU_CHECK_OK ditto http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager-test.cc@336 PS8, Line 336: CreateTestTable(); Here and below: Wrap this into ASSERT_OK() http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.h@1060 PS8, Line 1060: std::optional<rpc::RpcContext*> rpc Passing a pointer wrapped into std::optional doesn't make much sense unless it's necessary to distinguish between nullptr and the absence of a pointer at all. However, in the related code it's always checked for the presence of the pointer it's being a non-null. Also, creating std::optional objects is not free -- it takes some CPU cycles. With that, why not to pass just a pointer here instead? http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.h@1391 PS8, Line 1391: const std::optional<rpc::RpcContext*>& rpc_opt This is a bit funny: in all the other places, the wrapper for the pointer is passed by value, but here it's passed by reference. Why is the difference? If just a pointer was passed around, there would be no chance for ambiguity. http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@a3362 PS8, Line 3362: Maybe, it's better to leave it here but remove similar assert from AlterTableWithUser() and AlterTable() methods? http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2231 PS8, Line 2231: leader_lock_.AssertAcquiredForReading(); Why not to move this into CreateTableHelper() instead? That way it's less code and, most importantly, less bugs if anybody is about to start using CreateTableHelper() elsewhere. http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2730 PS8, Line 2730: leader_lock_.AssertAcquiredForReading(); Why not to move this into DeleteTableHelper() instead? http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2737 PS8, Line 2737: from $0 Is there a way to provide the information on the source address where the request came from? That's valuable for auditing and troubleshooting purposes to have this in the logs. http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@2739 PS8, Line 2739: leader_lock_.AssertAcquiredForReading(); Why not to move this into DeleteTableHelper() instead? http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@3501 PS8, Line 3501: leader_lock_.AssertAcquiredForReading(); Why not to move this into AlterTableHelper() instead? http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@3511 PS8, Line 3511: leader_lock_.AssertAcquiredForReading(); ditto http://gerrit.cloudera.org:8080/#/c/22420/8/src/kudu/master/catalog_manager.cc@6854 PS8, Line 6854: MonoTime::Max(); I don't think waiting insanely long makes any sense here. There should be some sane timeout interval. Maybe, introduce a new dedicated flag for this timeout with default value of, say, 30 seconds? -- To view, visit http://gerrit.cloudera.org:8080/22420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If52359603e3aa8af2dedc41ecc5eb78d03151fe5 Gerrit-Change-Number: 22420 Gerrit-PatchSet: 8 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Thu, 13 Feb 2025 04:57:44 +0000 Gerrit-HasComments: Yes
