Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21823 )
Change subject: KUDU-3608: REST API for Metadata Management ...................................................................... Patch Set 25: (24 comments) Since Attila explicitly asked me not to jump into code reviews of WIP gerrit items, I wasn't following this changelist for some time. However, it seems I missed when WIP tag was removed :) There were also a few question I needed to respond, so here you go. This patch has already been committed into the git repo, but maybe consider addressing this feedback in a separate changelist follow-up patch, if it makes sense to you. Thank you for working on this! http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog-test.cc File src/kudu/master/rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog-test.cc@537 PS34, Line 537: "name": "int_val" : nit for here and elsewhere where the status isn't used elsewhere: it might shortened to ASSERT_OK(client_->OpenTable(kTableName, &table)); http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h File src/kudu/master/rest_catalog_path_handlers.h: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h@34 PS34, Line 34: // Web page support for the master. nit: consider making this class 'final' http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h@36 PS34, Line 36: nit: consider adding DCHECK_NOT_NULL wrapper around 'master' http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.h@38 PS34, Line 38: nit: if the destructor is empty, consider using ~RestCatalogPathHandlers() = default; http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc File src/kudu/master/rest_catalog_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@24 PS34, Line 24: #include "google/protobuf/util/json_util.h" nit: change quotes to '<' and '>' and re-arrange the lines around to be sorted alphabetically http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@71 PS34, Line 71: ()->GetTableIn Why is this passed by pointer if 'jw' isn't changing in this method? http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@73 PS34, Line 73: Why is this passed by pointer if 'status_code' isn't changing in this method? http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@90 PS34, Line 90: What if 'table_id' isn't there? There will be an exception thrown, and the process will crash. It's not acceptable. http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@108 PS34, Line 108: For here and elsewhere: why not to return InternalServerError or other server-side error code then? I don't think returning ServiceUnavailable regardless of exact Status is in-line with the semantics of HTTP 503 code: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/503 I'd think that returning HttpStatusCode::ServiceUnavailable would make sense if status.IsServiceUnavailable() were true. In other words, there should be a meaningful conversion from Status into the server-level subset of HttpStatusCode enumeration. http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@146 PS34, Line 146: Http nit for here and elsewhere: remove the 'std::' prefix since there are corresponding 'using ...' directives in the beginning of this file already http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@322 PS34, Line 322: What if status is non-OK here? http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_path_handlers.cc@346 PS34, Line 346: If the method doesn't return anything but Status::OK(), maybe it should have been returning 'void' instead of 'Status'? http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h File src/kudu/master/rest_catalog_test_base.h: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@43 PS34, Line 43: What is this for? http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@50 PS34, Line 50: nit for here and elsewhere: I'd think of removing the 'kudu::' namespace prefix -- this code is already in the 'kudu' namespace and there isn't any ambiguity for compiler to find Status http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@64 PS34, Line 64: nit: this could be constexpr http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/rest_catalog_test_base.h@74 PS34, Line 74: : return tableCreator->Create(); http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@67 PS34, Line 67: nit for here and elsewhere in this test: introduce a const/constexpr static member field and use it instead http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@72 PS34, Line 72: nit: in C++ that's usually expressed as InternalMiniClusterOptions opts; http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@75 PS34, Line 75: nit: wrap this into std::move() to avoid extra copying http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@147 PS34, Line 147: This isn't a reliable way to check for OK status because error messages can contain 'OK' substring. If you want to check for a particular status, there are Status::ok() and other methods Status::IsXxx(), e.g. Status::IsNotFound(). Use those instead of checks like this. http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@206 PS34, Line 206: nit for here and elsewhere: the expected/reference value should come first -- that way it's much easier to comprehend the error message if {ASSERT,EXPECT}_EQ() assertion is ever triggered; you can try to parse this yourself for both options and see which one is easier to comprehend http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@220 PS34, Line 220: Here and elsewhere in case of non-OK status: add an assertion for the exact type of type of status/error returned (e.g., Status::IsNotFound()). http://gerrit.cloudera.org:8080/#/c/21823/34/src/kudu/master/spnego_rest_catalog-test.cc@229 PS34, Line 229: nit: please add a small text blurb to explain why this isn't enabled for macOS http://gerrit.cloudera.org:8080/#/c/21823/25/src/kudu/server/webserver.cc File src/kudu/server/webserver.cc: PS25: > These cases are already used in the code ... I could not find HttpStatusCode::NoContent, HttpStatusCode::GatewayTimeout in the code, at least as of PS25, but I might be missing something. Could you point where they are used in the code? > Could you clarify if there's a specific concern that warrants separating it? Smaller, logically separated patches makes it much easier to reason about, especially if changing something that a thing on its own, like in this particular case. Logically separated patches are easier for the author to revv, for code maintainers to cherry-pick into other branches, and for reviewers to reason about and provide feedback. It's also easier to assess what test coverage is needed, and implement tests that are adequate and meaningful. In other words, it's just common sense. -- To view, visit http://gerrit.cloudera.org:8080/21823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67f964c4f950edfde31772cafd5c3ed5d6b87413 Gerrit-Change-Number: 21823 Gerrit-PatchSet: 25 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-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Fri, 28 Mar 2025 17:39:24 +0000 Gerrit-HasComments: Yes
