[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 7: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 07 Mar 2020 00:00:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter has heard back from all pending backends and complete. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_updates_received_ flag in FilterState class and set it to true after filter received enough filter update from pending backends to proceed. If all_updates_received_ is true, then that runtime filter is considered as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Reviewed-on: http://gerrit.cloudera.org:8080/15308 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 3 files changed, 32 insertions(+), 15 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 8 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 7: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5458/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 19:08:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 19:08:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 7 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 19:08:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 6: Code-Review+1 Thanks Riza for the patch! The patch looks good to me after you added an additional input argument when calling Disable(). -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 18:39:46 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/15308 )
Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked
disabled
..
Patch Set 6:
(3 comments)
Hi Tim,
Thanks for your feedback. Patch set 5..6 should address your comment.
Let me know if there is something else I missed or can be improved.
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator-filter-state.h@100
PS4, Line 100: bool received_all_updates() const { return
all_updates_received_; }
> I'd consider making this an argument to Disable() and DisableAndRelease(),
Done
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator.cc@605
PS4, Line 605: // In case of remote filter, we might intentionally disable
the filter upon
> Maybe simplify to:
Done
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator.cc@1366
PS4, Line 1366: void Coordinator::FilterState::DisableAndRelease(
> nit: the coding style is usually to put conditions on a single line if they
Done. This is removed now since the flag setting is done through function
Disable().
--
To view, visit http://gerrit.cloudera.org:8080/15308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef
Gerrit-Change-Number: 15308
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 06 Mar 2020 17:42:30 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5444/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 16:43:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Hello Fang-Yu Rao, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15308 to look at the new patch set (#6). Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter has heard back from all pending backends and complete. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_updates_received_ flag in FilterState class and set it to true after filter received enough filter update from pending backends to proceed. If all_updates_received_ is true, then that runtime filter is considered as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef --- M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 3 files changed, 32 insertions(+), 15 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15308/6 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 6 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5443/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 15:59:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Hello Fang-Yu Rao, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15308 to look at the new patch set (#5). Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter has heard back from all pending backends and complete. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_updates_received_ flag in FilterState class and set it to true after filter received enough filter update from pending backends to proceed. If all_updates_received_ is true, then that runtime filter is considered as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef --- M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 3 files changed, 29 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15308/5 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5435/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 06 Mar 2020 00:57:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15308 )
Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked
disabled
..
Patch Set 4:
(3 comments)
This is looking good. I have a bit of nit-picky feedback, nothing serious. This
is mainly about fitting with the codebase's existing style and making it
slightly easier to maintain in future.
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator-filter-state.h@100
PS4, Line 100: void set_all_updates_received() { all_updates_received_ =
true; }
I'd consider making this an argument to Disable() and DisableAndRelease(),
because at each callsite a programmer already needs to think about whether they
should also be calling set_all_updates_received(). If it's a non-optional
argument this forces them to think about it.
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator.cc@605
PS4, Line 605: if (state.disabled()) {
Maybe simplify to:
row.push_back(state.received_all_updates() || !state.disabled());
This is optional, I just have a strong bias towards minimising the number of
control flow blocks, I find it easier to follow the logic of the function.
http://gerrit.cloudera.org:8080/#/c/15308/4/be/src/runtime/coordinator.cc@1366
PS4, Line 1366: if (!disabled()) {
nit: the coding style is usually to put conditions on a single line if they fit
if (!disabled()) set_all_updates_received();
Also, this is totally optional, but it might be slightly more readable if you
avoided the double negative in these conditions by defining an enabled()
accessor method that just returned !disabled().
--
To view, visit http://gerrit.cloudera.org:8080/15308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef
Gerrit-Change-Number: 15308
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 06 Mar 2020 00:36:32 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/15308 )
Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked
disabled
..
Patch Set 4:
(7 comments)
Thanks Fang-Yu, the following changes address your comments.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator-filter-state.h@99
PS3, Line 99: bool received_all_updates() const { return
all_updates_received_; }
: void set_all_updates_received() { all_updates_received_ = true;
> nit: Is it better to change the names of the functions to received_all_upda
Done
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator-filter-state.h@162
PS3, Line 162: coordi
> nit: Is it better to change it from 'filter' to 'the coordinator'?
Done
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1266
PS3, Line 1266:
> nit: The filter may have already been disabled.
Removed for shorter comment.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1267
PS3, Line 1267: >Disable()
> nit: the coordinator receives an.
Removed for shorter comment.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1268
PS3, Line 1268:
> nit: updates have.
Done, moved to ApplyUpdate().
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1269
PS3, Line 1269: arams.mutable_dst_qu
> nit: Change it to 'all_updates_received_' if you decide to change the name
Done, also moved to ApplyUpdate().
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1270
PS3, Line 1270: rpc_params.set_filter_id(params.filter_id());
> Taking a closer look at Coordinator::FilterState::ApplyUpdate(), I was wond
Make sense. So all flag flipping happen within ApplyUpdate().
Moved this along the comment under ApplyUpdate().
--
To view, visit http://gerrit.cloudera.org:8080/15308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef
Gerrit-Change-Number: 15308
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 06 Mar 2020 00:21:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Hello Fang-Yu Rao, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15308 to look at the new patch set (#4). Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter has heard back from all pending backends and complete. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_updates_received_ flag in FilterState class and set it to true after filter received enough filter update from pending backends to proceed. If all_updates_received_ is true, then that runtime filter is considered as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef --- M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 3 files changed, 25 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15308/4 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 4 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Fang-Yu Rao has posted comments on this change. (
http://gerrit.cloudera.org:8080/15308 )
Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked
disabled
..
Patch Set 3:
(7 comments)
Hi Riza, thanks for addressing my previous comments! The patch looks good to
me. I only left some very minor comments.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator-filter-state.h
File be/src/runtime/coordinator-filter-state.h:
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator-filter-state.h@99
PS3, Line 99: bool received_all_update() const { return all_update_received_;
}
: void set_all_update_received() { all_update_received_ = true; }
nit: Is it better to change the names of the functions to
received_all_updates() and set_all_updates_received(), respectively?
Also is it better to change 'all_update_received_' to 'all_updates_received_'?
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator-filter-state.h@162
PS3, Line 162: filter
nit: Is it better to change it from 'filter' to 'the coordinator'?
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1266
PS3, Line 1266: Filter may already disabled
nit: The filter may have already been disabled.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1267
PS3, Line 1267: it receive
nit: the coordinator receives an.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1268
PS3, Line 1268: update has
nit: updates have.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1269
PS3, Line 1269: all_update_received_
nit: Change it to 'all_updates_received_' if you decide to change the name of
this variable.
http://gerrit.cloudera.org:8080/#/c/15308/3/be/src/runtime/coordinator.cc@1270
PS3, Line 1270: if (!state->disabled()) state->set_all_update_received();
Taking a closer look at Coordinator::FilterState::ApplyUpdate(), I was
wondering if it would be more intuitive to call set_all_update_received() at
the end of ApplyUpdate().
Specifically, after
https://github.com/apache/impala/blob/master/be/src/runtime/coordinator.cc#L1352-L1354,
we additionally check whether or not it is true that 'pending_count_' is equal
to 0 and '!disabled()' evaluates to true. If so, we call
set_all_update_received().
Let me know if I have missed something. I am fine with both approaches.
--
To view, visit http://gerrit.cloudera.org:8080/15308
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef
Gerrit-Change-Number: 15308
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Fang-Yu Rao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 05 Mar 2020 22:27:25 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5431/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Mar 2020 18:32:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 3: (1 comment) Patch set 3 submitted. This change will consider a valid always_true filter as enabled. On the contrary, filter that becomes always_true due to RPC failure upon reading update from backend is considered as disable. http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc@1268 PS2, Line 1268: // still enabled at this point, it means all filter update has been successfully > I think I agree with your assessments for point 1 to 5. Done I go with this proposed change and flip the flag for case 1 and 4. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 05 Mar 2020 17:56:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Hello Fang-Yu Rao, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15308 to look at the new patch set (#3). Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter has heard back from all pending backends and complete. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_update_received_ flag in FilterState class and set it to true after filter received enough filter update from pending backends to proceed. If all_update_received_ is true, then that runtime filter is considered as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef --- M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 3 files changed, 25 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15308/3 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 2: (1 comment) Hi Fang-Yu, thanks for your feedback. I replied to your comment below. http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc@1268 PS2, Line 1268: state->set_all_update_received(); > I think by calling "state->set_all_update_received();" here (after the call I think I agree with your assessments for point 1 to 5. Regarding your last concern, I now realize that if ApplyUpdate() caught in RPC failure, the filter still get published as always_true filter. So it is probably better to move this line before Disable() but with precondition that the filter is not yet disabled, like this: if (!state->disabled()) state->set_all_update_received(); state->Disable(); Thus, case number 2 and 3, where there is an RPC error in ApplyUpdate(), will not be marked as complete runtime filter. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 21:18:51 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 2: (1 comment) Hi Riza, I have added an additional comment. Sorry for any confusion caused. http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc@1268 PS2, Line 1268: state->set_all_update_received(); > I just realized that Disable() is also called in DisableAndRelease(), which I think by calling "state->set_all_update_received();" here (after the call to state->ApplyUpdate()), we also mask the cases where there were some errors happening during the transmission of the runtime filter. I was wondering whether or not we need to add an additional variable to help us distinguish these 2 cases (some error occurred v.s. a set of filters that could be used to construct a correct aggregated filter have already been received). -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 19:47:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 2: (1 comment) Hi Riza, I left some comments about when we should call set_all_update_received() according to my current understanding. Let me know how you think about them. Thanks! http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/15308/2/be/src/runtime/coordinator.cc@1268 PS2, Line 1268: state->set_all_update_received(); I just realized that Disable() is also called in DisableAndRelease(), which may be invoked in different scenarios. In some scenarios, I think we may need to call set_all_update_received(). In what follows, I list those places where DisableAndRelease() is called and briefly describe the reason why DisableAndRelease() is called according to my current understanding. 1. https://github.com/apache/impala/blob/master/be/src/runtime/coordinator.cc#L1304 The Bloom filter the Coordinator just received represents an array of all 1's, i.e., it is an always true filter. In this case the Coordinator does not have to receive other possibly in-flight Bloom filters because the aggregated Bloom filter would not change anyway. Thus, in this case, we may call set_all_update_received() because a set of filters that are used to construct a correct Bloom filter have been received. 2. https://github.com/apache/impala/blob/master/be/src/runtime/coordinator.cc#L1319 An error occurred because the Coordinator is not able to retrieve the contents in the Bloom filter via context->GetInboundSidecar(). In this case, we do NOT call set_all_update_received() because a correctly aggregated Bloom filter cannot be constructed. 3. https://github.com/apache/impala/blob/master/be/src/runtime/coordinator.cc#L1327 Similarly, an error occurred due to another reason. In this case, we do NOT call set_all_update_received(). 4. https://github.com/apache/impala/blob/master/be/src/runtime/coordinator.cc#L1343 The Min/max filter the Coordinator just received represents all the values in the domain so no further aggregation is required. In this case, we may call set_all_update_received() because a set of filters that are used to construct a correct Bloom filter have been received. 5. https://github.com/apache/impala/blob/master/be/src/runtime/coordinator.cc#L1114 DisableAndRelease() is called in Coordinator::ReleaseExecResources(), which is called when the execution state of this query is transitioning to the terminal state. I think in this case, we do NOT have to call set_all_update_received() for the following reasons. i) In the case when a set of filters to form the correct aggregated filter have been received. set_all_update_received() would have already been called so that we do NOT have call it here. ii) In the case when a correctly aggregated filter cannot be constructed, set_all_update_received() will NOT be called and of course we do not have to call set_all_update_received() here. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 19:30:54 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5419/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 18:18:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 2: (1 comment) Patch set 2 submitted. We say that remote runtime filter is enabled if it successfully received all filter updates from pending backends, regardless if there is an RPC failure later in publish phase. I try to avoid calling this flag "is_complete_" because this flag only applies to remote filter case. http://gerrit.cloudera.org:8080/#/c/15308/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15308/1/be/src/runtime/coordinator-backend-state.cc@671 PS1, Line 671: state->get_publish_filter_done_cv().notify_one(); > Agree, I will move the flag right after the Disable() call in UpdateFilter( Done -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 17:44:03 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Hello Fang-Yu Rao, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15308 to look at the new patch set (#2). Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter has heard back from all pending backends and complete. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_update_received_ flag in FilterState class and set it to true right after Disable() call in UpdateFilter(). If all_update_received_ is true, then that runtime filter is considered as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef --- M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 3 files changed, 15 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15308/2 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/15308/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15308/1/be/src/runtime/coordinator-backend-state.cc@671 PS1, Line 671: state->set_all_publish_complete(); > On first look, it seemed weird that the runtime filter flips from not disab Agree, I will move the flag right after the Disable() call in UpdateFilter(), and rename this flag accordingly. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 03:28:32 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 1: (1 comment) This seems to solve the problem but I think the implementation could be either simpler or better explained. I left comments inline. http://gerrit.cloudera.org:8080/#/c/15308/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15308/1/be/src/runtime/coordinator-backend-state.cc@671 PS1, Line 671: state->set_all_publish_complete(); On first look, it seemed weird that the runtime filter flips from not disabled to disabled to disabled again in the filter table. I guess I also don't exactly understand what the intent is. I had initially thought that it might be to show filters where PublishFilter() failed as disabled, but it looks like set_all_publish_complete() is called even if there's an RPC error. One idea that might simplify things is if in UpdateFilter() we marked the filter as complete at the same time as calling Disable(). -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Fang-Yu Rao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 04 Mar 2020 01:52:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15308 ) Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/5358/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Thu, 27 Feb 2020 16:43:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15308 Change subject: IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled .. IMPALA-8674: fix bug where REMOTE runtime filter always marked disabled When a runtime filter has remote target, coordinator will Disable the FilterState upon arrival of the last filter update to prevent another update towards that filter. As consequence, such runtime filter will always be displayed as disabled in runtime profile (Enabled column is equal to false in Final filter table), when in reality the runtime filter is complete and successfully published to all remote targets. The Enabled column should correctly distinguish between failed runtime filter vs complete runtime filter. To do so, we add all_publish_complete_ flag in FilterState class and set it to true upon completion of the final runtime filter publish. If all_publish_complete_ is true, then mark that runtime filter as enabled. Testing: - Add row regex in runtime_filters.test, query 6, to verify REMOTE runtime filter is marked as enabled in final filter table - Run and pass test_runtime_filters.py - Run and pass core tests Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef --- M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-filter-state.h M be/src/runtime/coordinator.cc M testdata/workloads/functional-query/queries/QueryTest/runtime_filters.test 4 files changed, 16 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/15308/1 -- To view, visit http://gerrit.cloudera.org:8080/15308 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I82a5a776103abd0a6d73336bebc65e22b4e13fef Gerrit-Change-Number: 15308 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto
