Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8070 )
Change subject: IMPALA-5908: Allow SET to unset modified query options. ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/impala-server.h@347 PS10, Line 347: TQueryOptions *server_default_query_options; nit: placement of * http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/impala-server.h@306 PS7, Line 306: struct SessionState { This "struct' is pretty out of control. I agree with your earlier comment that this should be a class, particularly since it has non-trivial locking rules. I don't think we need to do that cleanup now though. http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8070/7/be/src/service/query-options.cc@134 PS7, Line 134: NULL nullptr (we still have NULL all over the codebase but we're gradually switching). http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@a61 PS10, Line 61: Ahh we've seen this mistake a few times before. http://gerrit.cloudera.org:8080/#/c/8070/10/be/src/service/query-options.cc@83 PS10, Line 83: nit: placement of * http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/8070/10/common/thrift/ImpalaInternalService.thrift@86 PS10, Line 86: a what? http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py File tests/custom_cluster/test_set_and_unset.py: http://gerrit.cloudera.org:8080/#/c/8070/10/tests/custom_cluster/test_set_and_unset.py@25 PS10, Line 25: class TestSetAndUnset(CustomClusterTestSuite, HS2TestSuite): Thanks for adding this, clearly there was a gap in test coverage here. -- To view, visit http://gerrit.cloudera.org:8080/8070 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9 Gerrit-Change-Number: 8070 Gerrit-PatchSet: 7 Gerrit-Owner: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Matthew Jacobs <mjac...@apache.org> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 29 Sep 2017 00:34:21 +0000 Gerrit-HasComments: Yes
