Alexey Serbin 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. 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? 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, at least the essential ones. 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 debug logging level is less than 2. 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. Also, maybe match the VLOG level with the the if() clause above? 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? Keep in mind that in this is a code in the client library, and logging capabilities might be limited. 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? -- 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 31 Jul 2023 18:30:50 +0000 Gerrit-HasComments: Yes
