Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20270 )
Change subject: KUDU-3461 [client] Avoid impala crash by returning error if invalid tablet id found ...................................................................... Patch Set 8: (22 comments) http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@511 PS8, Line 511: session->SetTimeoutMillis(60000); Is this customization is really necessary? Please add a comment to explain why, otherwise remove this. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@513 PS8, Line 513: Status s = session->Flush(); Why to store the result into a variable here? Also, why not to check for the result? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@519 PS8, Line 519: CHECK Why not to use regular ASSERT here? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@522 PS8, Line 522: status() Please also add an assertion on the type of the Status. Checking for the error message is secondary -- the actual robust code verifies the kind of the returned Status objects to make decisions, not error strings. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@522 PS8, Line 522: errors[0] I guess there should be an assertion on non-emptiness of 'errors' before trying to access its first element. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10184 PS8, Line 10184: This The ? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10189 PS8, Line 10189: // 1. Using client_, create a table with a range partition Would it be enough to have just 2 client instances: one is used to create a range partition and insert a row in the partition, while the other one is used to drop the partition while the other instance isn't aware of this activity? Why to introduce a 3rd one? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10190 PS8, Line 10190: a new insert client a new client http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10196 PS8, Line 10196: TestClientMetacacheRecursion nit: you could use CURRENT_TEST_NAME() macro instead of this http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10201 PS8, Line 10201: (0-1) This notation is confusing. Please update to make sure the lower bound comes first, the upper bound comes second. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10213 PS8, Line 10213: KuduTableCreator::EXCLUSIVE_BOUND, : KuduTableCreator::INCLUSIVE_BOUND); As for the range boundary types, why not to use the parameters-by-default of KuduTableCreator::add_range_partition()? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10235 PS8, Line 10235: unique_ptr<KuduTableAlterer> alterer(client_ddl_ops->NewTableAlterer(table_name)); : alterer.reset(client_ddl_ops->NewTableAlterer(table_name)); Why to do allocate a TableAlterer instance twice, discarding the first instance upon calling reset()? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10237 PS8, Line 10237: lower_bound.reset(schema_.NewRow()); : ASSERT_OK(lower_bound->SetInt32("key", 0)); : upper_bound.reset(schema_.NewRow()); : ASSERT_OK(upper_bound->SetInt32("key", 1)); : alterer->DropRangePartition(lower_bound.release(), upper_bound.release()); : ASSERT_OK(alterer->Alter()); This doesn't seem to be the range created above at lines 10201 -- 10205. Where does this range come from? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10259 PS8, Line 10259: This The ? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10272 PS8, Line 10272: TEST_F(ClientTest, TestClientMetacacheInvalidation) { Instead of duplicating the majority of the code between these two new scenarios, consider either using parameterized test, or move the common code into a helper function to be re-used in both places. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10273 PS8, Line 10273: "TestClientMetacacheInvalidation" ditto: could use CURRENT_TEST_NAME() macro instead of this http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10275 PS8, Line 10275: (0-1) This notation is confusing. The code below works with (-1, 0] range partition. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10309 PS8, Line 10309: unique_ptr<KuduTableAlterer> alterer(client_ddl_ops->NewTableAlterer(table_name)); : alterer.reset(client_ddl_ops->NewTableAlterer(table_name)); I'm not sure this makes any sense. Why to do this double allocation? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/meta_cache.cc@105 PS8, Line 105: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/meta_cache.cc@541 PS8, Line 541: FLAGS_prevent_kudu_3461_infinite_recursion Maybe, wrap this into PREDICT_TRUE() since the flag is used only in the newly added test scenarios? http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/meta_cache.cc@554 PS8, Line 554: LOG(INFO) << "Tablet under process found to be invalid, table: " : << table_->name() : << " - old tablet id: " << tablet_->tablet_id() : << ", new tablet id: " << remote_tablet->tablet_id(); Here and elsewhere in messages: use strings::Substitute() instead for better code readability. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/meta_cache.cc@620 PS8, Line 620: unsuccessful nit: failed -- To view, visit http://gerrit.cloudera.org:8080/20270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia09cf6fb1b1d10f1ad13a62b5c863bcd1e3ab26a Gerrit-Change-Number: 20270 Gerrit-PatchSet: 8 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Comment-Date: Wed, 23 Aug 2023 21:59:51 +0000 Gerrit-HasComments: Yes
