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: (21 comments) http://gerrit.cloudera.org:8080/#/c/20270/8//COMMIT_MSG Commit Message: PS8: > I have not given this a thought. Would require investigation of current sca Yes, please -- it would be great. I suspect a similar problem might be there as well (maybe, manifesting itself in a different way, though) -- this patch addresses the issue only for the write patch, IIUC. At least it makes sense to make sure a similar scenario doesn't crash the client for the read/scan path, and the behavior scan operation in such a case is not too surprising :) http://gerrit.cloudera.org:8080/#/c/20270/8//COMMIT_MSG@31 PS8, Line 31: The last statement i.e. #8 causes impalad (connected to impala-shell) to crash : With this change, last statement query fails and throws "invalid tablet id" error. Is this still so as of PS10? http://gerrit.cloudera.org:8080/#/c/20270/8//COMMIT_MSG@36 PS8, Line 36: expect it it return error ditto http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/batcher.cc@506 PS10, Line 506: leRpcS nit: DML 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@522 PS8, Line 522: status() > Added assert on status type and removed error string check. I'd think _both_ the check for the Status type and the error message are needed here to be certain about exercising the exact control path, no? That would help to catch regressions by the newly introduced test scenario if by some reason InvalidArgument is returned in the same path for different reason (say, when the related code is modified in not appropriate way in the future). If you plan to have this helper function to be universal in the sense of catching Status::InvalidArgument() in other scenarios, maybe then allow for returning the error message from this helper function and checking for the expected pattern at the call site? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@193 PS10, Line 193: DECLARE_bool(prevent_kudu_3461_infinite_recursion); nit: please keep these lines from 140 to 193 to be alphabetically ordered http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@521 PS10, Line 521: // Check for only the first Does it make sense to make sure all the rows failed to be written? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@10216 PS10, Line 10216: e nit: DML (since you are using 'DDL' notation for the other client instance) http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@10226 PS10, Line 10226: t(nullptr, &client_inse Pass the parameter by reference, not by value? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@10257 PS10, Line 10257: nit: the DDL client ? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@10266 PS10, Line 10266: . Using a new DDL client (client_ddl_ops), drop the partit nit: could you add a small blurb to point to the fact that the tablet backing the same range will be a different entity, not the same one that backed the prior incarnation of this range, so the prior metacache entry for this range is no longer valid? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@10277 PS10, Line 10277: -1)); nit: the Status::Code enumeration and all its elements (e.g., kInvalidArgument) are private to the Status class; when referring to various types of Status, please use the public interface (e.g., Status::InvalidArgument(), etc.). http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/client-test.cc@10288 PS10, Line 10288: KuduTableCreator What happens if not enabling the thread-safe way of death tests? Is it crucial to have it in this scenario? Please add a comment to explain why this is crucial to have (if so). http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@542 PS10, Line 542: /* First check the local metacache for tablet */ Code style for here and below: please use C++-style comments (i.e. // ...), especially for single-line comments. http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@544 PS10, Line 544: Status fastpath_status = meta_cache_->FastLookupTabletByKey( : table_, tablet_->partition().begin(), : MetaCache::LookupType::kPoint, &remote_tablet); nit: seems like an incorrect indentation? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@621 PS10, Line 621: missing $1 placeholder? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@621 PS10, Line 621: nit: remove the extra space? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@1291 PS10, Line 1291: PartitionKey partition_key, : MetaCache::LookupType lookup_type, : scoped_refptr<RemoteTablet>* remote_tablet) { : return DoFastPathLookup(table, &partition_key, lookup_type, remote_tablet); : } : Why to introduce an extra layer of indirection for DoFastPathLookup(), especially if the newly introduced method/function used just once in the code? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@1315 PS10, Line 1315: entry.DebugString(table)); Maybe, introduce a static/constexp literal and use it to avoid duplicating the same built-in literal at lines 1315 and 1316? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@1476 PS10, Line 1476: missing $1 placeholder? http://gerrit.cloudera.org:8080/#/c/20270/10/src/kudu/client/meta_cache.cc@1476 PS10, Line 1476: nit: RPC -- 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 18:27:20 +0000 Gerrit-HasComments: Yes
