Jason Fehr 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) http://gerrit.cloudera.org:8080/#/c/20524/20/be/src/service/internal-server-test.cc File be/src/service/internal-server-test.cc: http://gerrit.cloudera.org:8080/#/c/20524/20/be/src/service/internal-server-test.cc@81 PS20, Line 81: const string seek_str = "\"state\""; > You could use ssize_t. I'm trying to decide if this will still work. It wil It will work since queries are in a subobject within the json structure. Thus, there will always be an opening curly brace that is not at position 0 of the contents_str. That being said, the code is better if I switch to using ssize_t and go back to using the orignal loop end condition of pos >= 0. I am making that switch in the next patch. http://gerrit.cloudera.org:8080/#/c/20524/20/be/src/service/internal-server-test.cc@83 PS20, Line 83: // If an opening curly brace is found, then the state field is missing. > Could also be ASSERT_NE. Done http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc File be/src/service/internal-server-test.cc: http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc@101 PS21, Line 101: // DO NOT provide a value for max_records that is less than 5. > Delete if 'max_records' is unused (see below) Good catch. 'max_records' is now used. http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc@105 PS21, Line 105: max_records > Is 'max_records' used? Could it be deleted? Good catch. 'max_records' is now used. http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc@121 PS21, Line 121: // Insert some products that have a last_sold time. > It seems a bit untidy that all this work is done in a constructor rather th In main code, I agree. Since this class is only used in tests, I like having all this code in the constructor so that it avoids an extra line of code when using this class. http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc@277 PS21, Line 277: query_results results = make_shared<vector<string>>(); > Unused I think Done http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc@410 PS21, Line 410: DatabaseTest db_test = DatabaseTest(impala_server_, "invalid1"); > Unused Done http://gerrit.cloudera.org:8080/#/c/20524/21/be/src/service/internal-server-test.cc@440 PS21, Line 440: thread_count > Is 'thread_count' used? It used to be, but not anymore. Deleted. -- 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: Wed, 06 Dec 2023 18:01:46 +0000 Gerrit-HasComments: Yes
