Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22744 )
Change subject: KUDU-3608 REST API Follow-up ...................................................................... Patch Set 5: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/22744/3/src/kudu/master/rest_catalog-test.cc@234 PS3, Line 234: ASSERT_TRUE(!s.ok()); > For REST API errors, any non-200 HTTP status code results in a RemoteError, Indeed: there might be either Status::RemoteError() or Status::TimedOut() here with current implementation of EasyCurl wrapper. One might check for s.IsRemoteError(), but checking for string patterns in the error response is sufficient, I guess. http://gerrit.cloudera.org:8080/#/c/22744/5/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/22744/5/src/kudu/master/rest_catalog_path_handlers.cc@87 PS5, Line 87: static HttpStatusCode GetHttpCodeFromStatus(const Status& status) { It seems this translation assumes the 'status' passed by callers must be non-OK. Does it make sense to add DCHECK(!s.ok()) to catch programming mistakes if somebody incorrectly uses this function later on? http://gerrit.cloudera.org:8080/#/c/22744/5/src/kudu/master/rest_catalog_path_handlers.cc@89 PS5, Line 89: AuthenticationRequired This might be a theme on its own (so, maybe consider addressing it in a separate changelist), but I just wanted to point at it while you are updating the related code. There is a difference between authentication and authorization, and unless that's Status coming out of SpnegoStep(), should this conversion map Status::NotAuthorized() into HTTP 403 Forbidden? (BTW, consider adding corresponding code into the HttpStatusCode enum). As I understand, in the context of REST API server, the authentication is performed during SPNEGO negotiation phase. After that, the authenticity of a caller is supposed to be established and known to the server when a particular REST API's method is invoked. So, for Status::NotAuthorized() returned from system catalog's methods later on, HTTP 403 seems to be a better mapping from the semantics if I read it correctly at https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status#client_error_responses What do you think? http://gerrit.cloudera.org:8080/#/c/22744/5/src/kudu/master/rest_catalog_path_handlers.cc@127 PS5, Line 127: if (!status.ok()) { : HttpStatusCode http_code = GetHttpCodeFromStatus(status); : RETURN_JSON_ERROR(jw, status.ToString(), resp->status_code, http_code); : } nit for here and elsewhere: consider introducing a macro similar to RETURN_JSON_ERROR_VAL and using it for this recurring pattern -- To view, visit http://gerrit.cloudera.org:8080/22744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3cdb14d5a932eb8d1ba209ca803731749117024e Gerrit-Change-Number: 22744 Gerrit-PatchSet: 5 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Tue, 22 Apr 2025 20:26:19 +0000 Gerrit-HasComments: Yes
