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 3: (23 comments) 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 "gen-cpp/data_stream_service.pb.h" alphabetize 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: #include "gen-cpp/data_stream_service.pb.h" put this with the includes above 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 "gen-cpp/data_stream_service.pb.h" put this up with the other impala-internal includes above http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.h@97 PS3, Line 97: UniqueIdPB* convertUniqueId(const TUniqueId& tqid); remove this 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: // need this to invoke get proxy remove this http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@25 PS3, Line 25: whitespace can be removed http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@43 PS3, Line 43: #include "service/control-service.h" : #include "service/data-stream-service.h" : #include "exec/kudu-util.h" move these up with the includes above common/names.h has a bunch of 'using's in it, so we always import it last so they don't affect the other files being imported http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@110 PS3, Line 110: UpdateFilterResultPB res; move this declaration down to just above where its actually used http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@122 PS3, Line 122: Status rpc_status; You can just declare this on the same line its set, i.e. 'kudu::Status rpc_status = proxy->...' on line 125 http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@125 PS3, Line 125: FromKuduStatus since we're not returning this status or anything, we can just keep it as a kudu::Status. It will still support 'ok()', for the logging you can use 'ToString()' http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@127 PS3, Line 127: proxy->UpdateFilterPB not ok: Lets say something like "UpdateFilter() failed: " http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@128 PS3, Line 128: return; unneccesary http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@137 PS3, Line 137: should just be 4 spaces like it was before 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 http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@170 PS3, Line 170: UniqueIdPB* qidpb = new UniqueIdPB; instead of using 'new' here and 'set_allocated' below, it would be cleaner to pass params.mutable_query_id() in as the second param to TUniqueIdToUniqueIdPB() http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@182 PS3, Line 182: TNetworkAddress const TNetworkAddress& (saves a copy) http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@184 PS3, Line 184: 4 spaces http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.cc@208 PS3, Line 208: // memory will be allocated inside InitPB remove this 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 http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/timestamp-value.h@189 PS3, Line 189: valuepb typo 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: virtual void UpdateFilter(const UpdateFilterParamsPB *req, Could you add some brief comments on these? You can just copy the comments that were removed from ImpalaInternalService.thrift if you want 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 "service/impala-server.h" : : #include "gen-cpp/data_stream_service.pb.h" : #include "gen-cpp/data_stream_service.proxy.h" : #include "runtime/query-state.h" nit: all of these includes can be alphabetized into the same block http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/service/data-stream-service.cc@132 PS3, Line 132: return I think you'll also need to call RespondRpc() in this case. (you can flip the check to only call PublishFilter() if qs != nullptr and then only have one call to RespondRpc()) -- 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: 3 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, 19 Jul 2019 23:52:03 +0000 Gerrit-HasComments: Yes
