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 5: (23 comments) I have addressed most of Thomas' previous comments. Will be currently working on incorporating sidecar. Thanks. http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/decimal-value.h File be/src/runtime/decimal-value.h: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/decimal-value.h@27 PS3, Line 27: #include "runtime/types.h" > alphabetize Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/query-state.h@36 PS3, Line 36: > put this with the includes above Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.h@31 PS3, Line 31: #include <boost/unordered_map.hpp> > put this up with the other impala-internal includes above Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@24 PS3, Line 24: > remove this Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@25 PS3, Line 25: #include "gen-cpp/data_stream_service.proxy.h" > whitespace can be removed Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@43 PS3, Line 43: : #include "common/names.h" : > move these up with the includes above Thanks for the explanation for common/names.h! http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@110 PS3, Line 110: Status get_proxy_status = > move this declaration down to just above where its actually used Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@122 PS3, Line 122: UpdateFilterResult > You can just declare this on the same line its set, i.e. 'kudu::Status rpc_ Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@125 PS3, Line 125: < "UpdateFilte > since we're not returning this status or anything, we can just keep it as a Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@127 PS3, Line 127: > Lets say something like "UpdateFilter() failed: " Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@128 PS3, Line 128: > unneccesary Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@137 PS3, Line 137: bool has > should just be 4 spaces like it was before Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@169 PS3, Line 169: ( > remove this, the formatting was already right Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@170 PS3, Line 170: BloomFilter::ToProtobuf(bloom_fil > instead of using 'new' here and 'set_allocated' below, it would be cleaner Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@182 PS3, Line 182: RuntimeFilterB > const TNetworkAddress& Thanks for pointing this out! http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@184 PS3, Line 184: (closed > 4 spaces Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@208 PS3, Line 208: bloom_filters_.push_back(bloom_filter); > remove this Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/timestamp-value.h File be/src/runtime/timestamp-value.h: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/timestamp-value.h@176 PS3, Line 176: void ToColumnValuePB(ColumnValuePB* pvalue) const { > copy the comment from ToTColumnValue Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/timestamp-value.h@189 PS3, Line 189: value_p > typo Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/service/data-stream-service.h File be/src/service/data-stream-service.h: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/service/data-stream-service.h@66 PS3, Line 66: /// Called by fragment instances that produce local runtime filters to deliver them to > Could you add some brief comments on these? You can just copy the comments Thanks for the suggestion. It is done. http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/service/data-stream-service.cc File be/src/service/data-stream-service.cc: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/service/data-stream-service.cc@36 PS3, Line 36: #include "util/memory-metrics.h" : #include "util/parse-util.h" : #include "testutil/fault-injection-util.h" : #include "service/impala-server.h" : > nit: all of these includes can be alphabetized into the same block Done http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/service/data-stream-service.cc@132 PS3, Line 132: ) retu > I think you'll also need to call RespondRpc() in this case. (you can flip t Thanks for pointing this out! When "qs.get() == nullptr", I was wondering if we need to return some error message to the client side of this data stream service, e.g., "Status("qs.get() == nullptr")" or we need to define some TErrorCode and then return it. http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-server.h@601 PS2, Line 601: friend class ControlService; > remove this change 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: 5 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: Sat, 20 Jul 2019 19:24:33 +0000 Gerrit-HasComments: Yes
