Ashwani Raina 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: (24 comments) Thank you Alexey, for detailed review. http://gerrit.cloudera.org:8080/#/c/20270/8//COMMIT_MSG Commit Message: PS8: > Do scan token generation and usage of them in scan operations are affected I have not given this a thought. Would require investigation of current scan token code. I can create a jira ticket for that and use that to investigate (and add a test if applicable). http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/batcher.cc@505 PS8, Line 505: if (result.status.IsInvalidArgument()) { : result.result = RetriableRpcStatus::NON_RETRIABLE_ERROR; : return result; : } > Please add a comment to explain what sort of errors this clause is about to Done 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 Won't make much sense to set timeout here. Removed. Thanks for pointing out. 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 t Done. 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? Done. 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 Added assert on status type and removed error string check. 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 tr Done http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10184 PS8, Line 10184: This > The ? Done 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 Done 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 Done 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 Good point. Done. 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 com Replaced with [0, 1) to avoid confusion here and everywhere else in this change. 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 o Removed this to use default parameters. 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 inst Removed reset(). Thanks for pointing out. 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. W Above comment at 10201 was confusing and I was using custom parameters. Rectified the comment with correct range i.e. [0, 1) and using default parameters to AddRangePartition everywhere now. http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/client-test.cc@10259 PS8, Line 10259: This > The ? Done 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 scena Added a separate helper function in a derived class for these type of tests. 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 Done 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 partit Fixed. Using [0, 1) and default parameters everywhere. 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? Removed reset(). Thanks for pointing out. 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 Done 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 new Good point! Done. 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 bette Done http://gerrit.cloudera.org:8080/#/c/20270/8/src/kudu/client/meta_cache.cc@620 PS8, Line 620: unsuccessful > nit: failed Done -- 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: Fri, 25 Aug 2023 00:18:56 +0000 Gerrit-HasComments: Yes
