Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC ...................................................................... Patch Set 12: (28 comments) Please review the updated patch set revised according to your previous comments. Thank you very much! http://gerrit.cloudera.org:8080/#/c/13882/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13882/11//COMMIT_MSG@11 PS11, Line 11: Having incorporated sidecar on the aggregation stage when > This sort of stuff shouldn't be included in the commit message Thanks! I have removed this. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h File be/src/runtime/backend-client.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@19 PS11, Line 19: #define IMPALA_BACKEND_CLIENT_H > Should this file be deleted after this change ? I recall that Thomas once mentioned that we could file a follow-up JIRA to delete unnecessary code after porting the runtime filter to KRPC since it would be easier for reviewers to review the patch set. Please let me know how I should proceed. Thanks! http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@25 PS11, Line 25: #include "gen-cpp/ImpalaInternalService.h" : > try not to leave commented out code in patches you submit for review Sure. I have removed these two lines. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@37 PS11, Line 37: : > try not to include unrelated formatting changes in patches you submit for r Sure. I have excluded these unrelated changes. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/backend-client.h@52 PS11, Line 52: Sure. I have removed them. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@45 PS11, Line 45: #include "util/scope-exit-trigger.h" : #include "util/uid-util.h" > nit: alphabetize Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@539 PS11, Line 539: DCHECK(ProtoToQueryId(rpc_params.dst_query_id()) == query_ > Since this is only used once, you can just put the call inside the DCHECK. Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@551 PS11, Line 551: not a query-wide error - the remot > This is wrong, see: https://gerrit.cloudera.org/#/c/13818/ (you'll just ne Thanks for pointing this out! I have fixed this. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@555 PS11, Line 555: > Drop this. GetProxy() is not something that we'll probably ever retry. If y Thanks for the explanation. I have dropped this. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@556 PS11, Line 556: > WARNING Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@566 PS11, Line 566: > WARNING Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-backend-state.cc@563 PS11, Line 563: controller.Reset(); : } : } : : Coordinator::BackendState::InstanceStats::InstanceStats( : const FInstanceExecParams& exec_params, FragmentStats* fragment_stats, : ObjectPool* obj_pool) > Let's keep it like it was before an not retry. We may someday want to consi Thanks very much for the detailed explanation! I have removed this loop to keep the code like it was before. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-filter-state.h File be/src/runtime/coordinator-filter-state.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator-filter-state.h@68 PS11, Line 68: & > I think we can avoid making this change, see my comment in coordinator.cc Thanks for pointing this out. I have made the change accordingly. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.h@32 PS11, Line 32: #include "gen-cpp/data_stream_service.pb.h" > I don't think you need to import control_service here Indeed. It has been removed. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@926 PS11, Line 926: lock_guard<SpinLock> l(backend_states_init_lock_); > no need to add more whitespace Thanks. It has been removed. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@976 PS11, Line 976: > remove this Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@977 PS11, Line 977: if (filter_updates_received_->value() == 0) { > I think we can swap this around, i.e. aggregated_filter.Swap(rpc_params.mut Thanks for pointing this out. I have made the change accordingly. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@992 PS11, Line 992: if (target.is_local) continue; : target_fragment_idxs.insert(target.fragment_idx); : } > This can be simplified to : Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@998 PS11, Line 998: BloomFilterPB& aggregated_filter = state->bloom_filter(); > remove this Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@1008 PS11, Line 1008: } > The formatting was correct before, see the output of clang-format Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@1040 PS11, Line 1040: rst_arrival_time_ == 0L) { > update this Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/runtime/coordinator.cc@1066 PS11, Line 1066: > formatting Done http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/data-stream-service.cc File be/src/service/data-stream-service.cc: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/data-stream-service.cc@136 PS11, Line 136: Substitute("Query Stat > Is it a case in which it's legitimate for query state to be not found here Thanks for pointing this out. I have modified the error message accordingly. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.h File be/src/service/impala-internal-service.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.h@18 PS11, Line 18: #ifndef IMPALA_SERVICE_IMPALA_INTERNAL_SERVICE_H > Shouldn't this file be deleted after this change ? I recall that Thomas once mentioned that we could file a follow-up JIRA to delete unnecessary code after porting the runtime filter to KRPC since it would be easier for reviewers to review the patch set. Please let me know how I should proceed. Thanks! http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.cc File be/src/service/impala-internal-service.cc: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-internal-service.cc@36 PS11, Line 36: ImpalaInternalService::ImpalaInternalService() { > Same question. Shouldn't this file be deleted after this change ? Also, we I recall that Thomas once mentioned that we could file a follow-up JIRA to delete unnecessary code after porting the runtime filter to KRPC since it would be easier for reviewers to review the patch set. Please let me know how I should proceed. Thanks! http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/service/impala-server.h@342 PS11, Line 342: const TPingImpalaHS2Servic > Stale comment. ImpalaInternalService should not exist anymore after this ch Thanks for pointing this out. I have removed this comment. http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/13882/11/be/src/util/bloom-filter.h@79 PS11, Line 79: multiple times. > Stale comment Done http://gerrit.cloudera.org:8080/#/c/13882/11/common/protobuf/common.proto File common/protobuf/common.proto: http://gerrit.cloudera.org:8080/#/c/13882/11/common/protobuf/common.proto@43 PS11, Line 43: This is a union over all possible return types > nit: capitalize and period Done -- To view, visit http://gerrit.cloudera.org:8080/13882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a Gerrit-Change-Number: 13882 Gerrit-PatchSet: 12 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Fri, 02 Aug 2019 05:31:54 +0000 Gerrit-HasComments: Yes
