Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/20524 )
Change subject: IMPALA-12426: Adds the backend InternalServer class. ...................................................................... Patch Set 21: (8 comments) Just some tidying up and questions http://gerrit.cloudera.org:8080/#/c/20524/21//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20524/21//COMMIT_MSG@11 PS21, Line 11: back end Nit: backend http://gerrit.cloudera.org:8080/#/c/20524/21//COMMIT_MSG@12 PS21, Line 12: user security Does this mean authentication? Does authorization still happen? What user is authorized? http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server-test.cc File be/src/service/internal-server-test.cc: http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server-test.cc@80 PS23, Line 80: // Back up from the query_id field until the state field is found. Nit: This is probably OK, but can we parse json using rapidjson which we already use? https://rapidjson.org/ http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server-test.cc@132 PS23, Line 132: int first_sold = sold(gen); Nit: Our tests run in a lot of places and so I find randomness scary. If we do get failures then randomness is annoying to debug http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server-test.cc@138 PS23, Line 138: if (i < sqls_count - 2) { Nit: this is ugly but I'm not sure how to improve. StrCat might make it slightly neater and consistent with the rest of the test. http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server-test.cc@219 PS23, Line 219: TEST(InternalServerTest, InvalidQueryOption) { I like how clean the API is now http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server-test.cc@434 PS23, Line 434: 4_expected_msg, r maybe just "expected_msg" http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server.h File be/src/service/internal-server.h: http://gerrit.cloudera.org:8080/#/c/20524/23/be/src/service/internal-server.h@76 PS23, Line 76: assoicated Spelling: associated -- To view, visit http://gerrit.cloudera.org:8080/20524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27686aa563fac87429657e4980b29b0da91eb9e1 Gerrit-Change-Number: 20524 Gerrit-PatchSet: 21 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 07 Dec 2023 01:58:29 +0000 Gerrit-HasComments: Yes
