Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12840 )
Change subject: tablet_service: improve error handling specificity ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12840/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12840/1//COMMIT_MSG@20 PS1, Line 20: order mode > ORDERED here, right? Done http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test-base.h File src/kudu/tserver/tablet_server-test-base.h: http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test-base.h@116 PS1, Line 116: void VerifyScanRequestFailure(const ScanRequestPB& req, > Maybe pass 'req' by value and std::move() from the two call-sites? There's no perf benefit to that because the RPC proxy currently takes a const ref. http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_server-test.cc@2433 PS1, Line 2433: // Protobuf will parse an unrecognized enum value as UNKNOWN_ORDER_MODE. > Nit: this is meant to rationalize why L2434 is passing UNKNOWN_ORDER_MODE i Done http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2160 PS1, Line 2160: [&] { *error_code = TabletServerErrorPB::INVALID_SNAPSHOT; }); > Couldn't you do this via the existing RETURN_NOT_OK_EVAL also? That's a good point. I wasn't sure it would work, but it turns out that a macro can treat an assignment as an argument so I'll just switch to that. http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2402 PS1, Line 2402: Status s = PickAndVerifyTimestamp(scan_pb, tablet, &tmp_snap_timestamp); > Seems like a good place to use RETURN_NOT_OK_PREPEND_{CALL,EVAL} :) not really, because of the CloneAndPrepend() http://gerrit.cloudera.org:8080/#/c/12840/1/src/kudu/tserver/tablet_service.cc@2428 PS1, Line 2428: : tablet::MvccSnapshot snap; : tablet::MvccManager* mvcc_manager = tablet->mvcc_manager(); > Can these be scoped within block on L2431? No, but we can get rid of the shorthand mvcc_manager so I'll go ahead and do that. -- To view, visit http://gerrit.cloudera.org:8080/12840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1e6f7b01336e78ea8f37cb17b2c4485138e23581 Gerrit-Change-Number: 12840 Gerrit-PatchSet: 1 Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Mon, 25 Mar 2019 17:54:25 +0000 Gerrit-HasComments: Yes
