Thomas Tauber-Marshall 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: (7 comments) Starting to really come together. Some high level comments about the sidecar stuff http://gerrit.cloudera.org:8080/#/c/13882/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13882/12//COMMIT_MSG@11 PS12, Line 11: Having incorporated sidecar on the aggregation stage when : the runtime filter is a Bloom filter. When writing commit messages, you should be thinking about it from the perspective of someone who comes along weeks or months from now and needs to know what your patch does without having any context about the process you went through to produce the patch. So, rather than phrasing it as "I've now updated the patch to use sidecars", which only makes sense to people who have the context that the patch was originally written without them, its better to phrase it as "This patch uses sidecars because...". Or in other words, replace this with something like: "To save on copying, the underlying data for bloom filters is transmitted via sidecar" http://gerrit.cloudera.org:8080/#/c/13882/12/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: http://gerrit.cloudera.org:8080/#/c/13882/12/be/src/util/bloom-filter.h@97 PS12, Line 97: static void ToProtobufWithSidecar(const BloomFilter* filter, I don't think there's any reason we need to have both implementations - in practice we should only ever be using this version, so we can just eliminate the other version and name this one ToProtobuf() http://gerrit.cloudera.org:8080/#/c/13882/12/be/src/util/bloom-filter.h@99 PS12, Line 99: UpdateFilterParamsPB* params If you move 'sidecar_idx' into the BloomFilterPB, I think we can just take a BloomFilterPB* here instead http://gerrit.cloudera.org:8080/#/c/13882/12/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/13882/12/be/src/util/bloom-filter.cc@19 PS12, Line 19: #include "kudu/rpc/rpc_controller.h" leave a blank line after the first import for the header file that directly corresponds to this file http://gerrit.cloudera.org:8080/#/c/13882/12/be/src/util/bloom-filter.cc@59 PS12, Line 59: Status BloomFilter::Init(const BloomFilterPB& protobuf) { I think this needs to be updated to handle the sidecar case http://gerrit.cloudera.org:8080/#/c/13882/12/common/protobuf/data_stream_service.proto File common/protobuf/data_stream_service.proto: http://gerrit.cloudera.org:8080/#/c/13882/12/common/protobuf/data_stream_service.proto@89 PS12, Line 89: optional bytes directory = 2; Lets eliminate this field and only allow sending bloom filters with sidecars http://gerrit.cloudera.org:8080/#/c/13882/12/common/protobuf/data_stream_service.proto@120 PS12, Line 120: optional int32 sidecar_idx = 5; Lets put this in BloomFilterPB, name it something like 'directory_sidecar_idx', and include a brief comment -- 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 17:11:17 +0000 Gerrit-HasComments: Yes
