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 <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 05 Mar 2020 22:27:25 +0000 Gerrit-HasComments: Yes
