Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13873 )
Change subject: IMPALA-8656: Re-factor PlanRootSink into blocking and buffered implementations ...................................................................... Patch Set 3: (6 comments) Mostly looks good, just some comments about the feature flag and some code cleanup we should do to match newer best practices in the codebase http://gerrit.cloudera.org:8080/#/c/13873/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13873/3//COMMIT_MSG@24 PS3, Line 24: * Since no new functionality has been added no tests were added Can you add a test that exercises the option to confirm that it doesn't crash when you run a query? We don't want user-toggleable options that can cause instability. I actually like this approach of doing the development behind a feature flag; just gotta make sure that we keep the code in a state where we could cut a release from it if needed. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h File be/src/exec/blocking-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h@18 PS3, Line 18: #ifndef IMPALA_EXEC_BLOCKING_PLAN_ROOT_SINK_H #pragma once, since that's what we're doing in new code http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/blocking-plan-root-sink.h@56 PS3, Line 56: virtual Status Send(RuntimeState* state, RowBatch* batch); Can you add override annotations - that's a good practice for new code. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h File be/src/exec/buffered-plan-root-sink.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h@19 PS3, Line 19: #define IMPALA_EXEC_BUFFERED_PLAN_ROOT_SINK_H #pragma once http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/exec/buffered-plan-root-sink.h@34 PS3, Line 34: virtual Status Send(RuntimeState* state, RowBatch* batch); Can you add override annotations - that's a good practice for new code. http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/13873/3/be/src/service/query-options.h@175 PS3, Line 175: ADVANCED This should be a DEVELOPMENT query option at least until it's ready. -- To view, visit http://gerrit.cloudera.org:8080/13873 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8786b1a9af68ab0a8a094970d8f955eb20d04bca Gerrit-Change-Number: 13873 Gerrit-PatchSet: 3 Gerrit-Owner: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 17 Jul 2019 17:39:46 +0000 Gerrit-HasComments: Yes
