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 2: (17 comments) I've still got a lot to go through, but wanted to post some comments for you to get started on http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.h File be/src/service/impala-internal-service.h: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.h@1 PS2, Line 1: // Licensed to the Apache Software Foundation (ASF) under one This whole file can be removed http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.cc File be/src/service/impala-internal-service.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.cc@1 PS2, Line 1: // Licensed to the Apache Software Foundation (ASF) under one This whole file can be removed 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: remove this change http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter-test.cc File be/src/util/bloom-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter-test.cc@31 PS2, Line 31: #include "gen-cpp/data_stream_service.pb.h" This can be moved up with the includes above it http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter.cc@208 PS2, Line 208: out->mutable_directory()->data() I don't think that you can use string::data() like this, see: http://www.cplusplus.com/reference/string/string/data/ In general its rare that using const_cast is the right thing to do I think what we want instead is to do it the same way as before, using '[0]'. http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter-test.cc File be/src/util/min-max-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter-test.cc@28 PS2, Line 28: #include "gen-cpp/data_stream_service.pb.h" This can be put with the block of includes right above it http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc File be/src/util/min-max-filter.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc@378 PS2, Line 378: if (in_min_val < out_min_val) out->mutable_min()->set_timestamp_val(in.min().timestamp_val()); > line too long (98 > 90) I don't remember if we talked about clang-format-diff yet, but if not you might want to look into it (though I think it'll give you the wrong advice here - if you move the body of the 'if' to the next line, make sure to add '{' and '}' around it) http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc@507 PS2, Line 507: #define DECI I think this was unintentional? http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/control_service.proto@274 PS2, Line 274: } please omit this from the patch http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto File common/protobuf/data_stream_service.proto: http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@82 PS2, Line 82: enum ImpalaInternalServiceVersionPB { This can be removed (and the corresponding fields in the messages below). When we first did this, the idea of versioning the internal rpcs made sense, in case we wanted to support stuff like rolling upgrades or running impalads of different versions in the same cluster. However, we haven't ever actually implemented anything like that (and probably won't any time soon) and it wouldn't be too hard to just add versions back in if we ever did want to, so we've just been omitting this when we port stuff over to krpc. http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@89 PS2, Line 89: required Lets make everything optional http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@103 PS2, Line 103: message ColumnValuePB { Put this in common.proto http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@127 PS2, Line 127: // UpdateFilter Remove this. Obviously you just copied this from ImpalaInternalService.thrift, but we don't have the same type of comment above the other messages already in this file, so its nice to be consistent (and a comment like this seems unnecessary anyways) http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@133 PS2, Line 133: // required in V1 remove this, here and below http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@150 PS2, Line 150: optional int64 receiver_latency_ns = 2; copy the comment from the equivalent field above http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@196 PS2, Line 196: rpc UpdateFilter(UpdateFilterParamsPB) returns (UpdateFilterResultPB); copy over the comments that were with these in ImpalaInternalService.thrift http://gerrit.cloudera.org:8080/#/c/13882/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/13882/2/common/thrift/ImpalaInternalService.thrift@705 PS2, Line 705: service ImpalaInternalService { : : } This can be removed -- 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: 2 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 17:08:26 +0000 Gerrit-HasComments: Yes
