Adar Dembo 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? 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? 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 instead of something more exotic like 123? If so, not sure I see the need; explicitly passing UNKNOWN_ORDER_MODE seems like a fine test case in its own right, and the name of the test suggests that this is what the test is going to do. 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? 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} :) 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? -- 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-Comment-Date: Sat, 23 Mar 2019 23:22:37 +0000 Gerrit-HasComments: Yes
