Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20270 )
Change subject: [client] Avoid impala crash by returning error if invalid tablet id found ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/20270/3//COMMIT_MSG Commit Message: PS3: > It seems a test that covers the updated functionality is missing. If you are talking about adding a unit test for this change, I don't think is an updated functionality. Without this change, impala daemon would crash if such a situation arises. With this fix, it would simply go on about its business after receiving an error from kudu client instead of going into infinite recursion. If there was a change in behaviour of queries issued, it would have fallen under the category where a unit test could validate such a thing. In addition, a test here would require both client where DDL ops are issued via java client and insert ops via c++ client. I am not sure if that is possible to achieve with an automated test. I gave it a little thought. Probably in the direction where control flags can be added to the c++ client that could mimic the scenario of selectively skipping the clearing of cache for step #7. So that when #8 is performed, it finds the invalid entry still present in cache thereby gets into a recursive loop. However, this kind of manipulation of code would be too much of a deviation from the actual case at hand due to no involvement of impala daemon, impala-shell and java client for that matter. Is there a way to invoke DDL ops via java client and insert ops via c++ client from a test? If there is, I can add that under a new test that covers the functionality at least from the perspective that subsequent insert op via c++ client succeeds. http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.h@441 PS3, Line 441: ) > nit: missing period in the end of the sentence? Done http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.h@442 PS3, Line 442: Status FastLookupTabletByKey(const KuduTable* table, > It would be great to document cases when this method returns non-OK status, Done http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.cc@465 PS3, Line 465: if (leader) { : VLOG(2) << "Client copy of tablet " << tablet_->tablet_id() : << " is fresh, leader uuid " << leader->permanent_uuid(); : } else { : VLOG(2) << "Client copy of tablet " << tablet_->tablet_id() : << " is fresh (no leader)."; : } > Consider wrapping this into VLOG_IS_ON() to avoid empty statements when deb Done http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.cc@513 PS3, Line 513: } else { : VLOG(2) << "Tablet " << tablet_->tablet_id() << ": No valid leader."; > Ditto. Done http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.cc@539 PS3, Line 539: LOG(INFO) << "Explicit fastpath lookup succeeded(maybe), proceed with callback, table: " : << table_->name(); > Why to log with with INFO? Changed to VLOG(2). http://gerrit.cloudera.org:8080/#/c/20270/3/src/kudu/client/meta_cache.cc@608 PS3, Line 608: LOG(INFO) << "Tablet lookup unsuccessful, tablet id: " << tablet_->tablet_id() > Why to log about this with LOG(INFO), especially in the client library? Changed to VLOG(2) -- 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: 3 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-Comment-Date: Tue, 01 Aug 2023 12:23:19 +0000 Gerrit-HasComments: Yes
