Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/18350 )
Change subject: WIP KUDU-3290 support duplication to kafka ...................................................................... Patch Set 2: (45 comments) http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/common/types-test.cc File src/kudu/common/types-test.cc: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/common/types-test.cc@138 PS2, Line 138: string slower, supper; > warning: multiple declarations in a single statement reduces readability [r Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/common/types.h File src/kudu/common/types.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/common/types.h@65 PS2, Line 65: const size_t size() const { return size_; } > warning: return type 'const size_t' (aka 'const unsigned long') is 'const'- reserve it http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/consensus/quorum_util.h File src/kudu/consensus/quorum_util.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/consensus/quorum_util.h@118 PS2, Line 118: bool ShouldAddDuplicator(const RaftConfigPB& config, > warning: function 'kudu::consensus::ShouldAddDuplicator' has a definition w Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/consensus/quorum_util.cc File src/kudu/consensus/quorum_util.cc: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/consensus/quorum_util.cc@541 PS2, Line 541: const unordered_set<string>& uuids_ignored_for_underreplication) { > warning: parameter 'uuids_ignored_for_underreplication' is unused [misc-unu Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h File src/kudu/duplicator/connector.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@46 PS2, Line 46: DuplicateMsg(const std::shared_ptr<tablet::WriteOpState>& _op_state, > warning: invalid case style for parameter '_op_state' [readability-identifi Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@47 PS2, Line 47: const tablet::RowOp* _row_op, > warning: invalid case style for parameter '_row_op' [readability-identifier Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@48 PS2, Line 48: const std::shared_ptr<Schema>& _schema_ptr, > warning: invalid case style for parameter '_schema_ptr' [readability-identi Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@49 PS2, Line 49: const std::string& _table_name) > warning: invalid case style for parameter '_table_name' [readability-identi Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@79 PS2, Line 79: LocalConnector() : Connector() {} > warning: initializer for base class 'kudu::duplicator::Connector' is redund Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@80 PS2, Line 80: virtual ~LocalConnector() = default; > warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [ Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@85 PS2, Line 85: RemoteConnector() : Connector() {} > warning: initializer for base class 'kudu::duplicator::Connector' is redund Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@86 PS2, Line 86: virtual ~RemoteConnector() = default; > warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [ Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@97 PS2, Line 97: KuduLocalConnector() : LocalConnector() {} > warning: initializer for base class 'kudu::duplicator::LocalConnector' is r Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@98 PS2, Line 98: virtual ~KuduLocalConnector() = default; > warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [ Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@99 PS2, Line 99: virtual Status Init() override { > warning: 'virtual' is redundant since the function is already declared 'ove Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/connector.h@103 PS2, Line 103: virtual Status WriteBatch(const std::vector<std::shared_ptr<DuplicateMsg>>& /* msgs */) override { > warning: 'virtual' is redundant since the function is already declared 'ove Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.h File src/kudu/duplicator/duplicator.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.h@57 PS2, Line 57: const consensus::OpId last_confirmed_opid() const { > warning: return type 'const consensus::OpId' is 'const'-qualified at the to Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.h@68 PS2, Line 68: // TODO use pointer to avoid copy > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.cc File src/kudu/duplicator/duplicator.cc: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.cc@54 PS2, Line 54: static bool ValidateDownstreamFlag(const char* flagname, const std::string& value) { > warning: parameter 'flagname' is unused [misc-unused-parameters] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.cc@66 PS2, Line 66: Duplicator::Duplicator(ThreadPool* thread_pool, const std::string& table_name) > warning: pass by value and use std::move [modernize-pass-by-value] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.cc@95 PS2, Line 95: // TODO metric_entity for duplicate_pool_ > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/duplicator.cc@106 PS2, Line 106: // TODO deal queue data > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h File src/kudu/duplicator/kafka/kafka_connector.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@28 PS2, Line 28: #include <stdint.h> > warning: inclusion of deprecated C++ header 'stdint.h'; consider using 'cst Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@69 PS2, Line 69: using std::map; > warning: using decl 'map' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@70 PS2, Line 70: using std::string; > warning: using declarations in the global namespace in headers are prohibit Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@140 PS2, Line 140: virtual Status InitPrepare() override { > warning: 'virtual' is redundant since the function is already declared 'ove Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@158 PS2, Line 158: virtual Status Init() override { > warning: 'virtual' is redundant since the function is already declared 'ove Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@209 PS2, Line 209: virtual Status WriteBatch( > warning: 'virtual' is redundant since the function is already declared 'ove Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@263 PS2, Line 263: virtual Status TestBasicClientApi(const std::string& msg) override { > warning: 'virtual' is redundant since the function is already declared 'ove Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.h@284 PS2, Line 284: virtual ~KafkaConnector() {} > warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [ Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc File src/kudu/duplicator/kafka/kafka_connector.cc: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@20 PS2, Line 20: using cppkafka::Configuration; > warning: using decl 'Configuration' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@21 PS2, Line 21: using cppkafka::Exception; > warning: using decl 'Exception' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@22 PS2, Line 22: using cppkafka::MessageBuilder; > warning: using decl 'MessageBuilder' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@23 PS2, Line 23: using cppkafka::Metadata; > warning: using decl 'Metadata' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@24 PS2, Line 24: using cppkafka::Producer; > warning: using decl 'Producer' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@26 PS2, Line 26: using std::make_shared; > warning: using decl 'make_shared' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@27 PS2, Line 27: using std::shared_ptr; > warning: using decl 'shared_ptr' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@29 PS2, Line 29: using std::vector; > warning: using decl 'vector' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/kafka_connector.cc@30 PS2, Line 30: using strings::Substitute; > warning: using decl 'Substitute' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/mock_kafka_connector.h File src/kudu/duplicator/kafka/mock_kafka_connector.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/mock_kafka_connector.h@33 PS2, Line 33: explicit MockProducer(const Configuration /* config */) : timeout_ms_(30000) {} > warning: the const qualified parameter #1 is copied for each invocation; co Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/duplicator/kafka/mock_kafka_connector.h@68 PS2, Line 68: void check_error(rd_kafka_resp_err_t error) { > warning: method 'check_error' can be made static [readability-convert-membe make it private. http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/integration-tests/duplication_with_kafka-itest.cc File src/kudu/integration-tests/duplication_with_kafka-itest.cc: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/integration-tests/duplication_with_kafka-itest.cc@24 PS2, Line 24: #include <stdint.h> > warning: inclusion of deprecated C++ header 'stdint.h'; consider using 'cst Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/integration-tests/duplication_with_kafka-itest.cc@25 PS2, Line 25: #include <stdlib.h> > warning: inclusion of deprecated C++ header 'stdlib.h'; consider using 'cst Done http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/tablet/ops/op_driver.cc File src/kudu/tablet/ops/op_driver.cc: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/tablet/ops/op_driver.cc@294 PS2, Line 294: if (op_->type() == consensus::FOLLOWER) { > warning: repeated branch in conditional chain [bugprone-branch-clone] temp http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/18350/2/src/kudu/tablet/tablet_replica.h@514 PS2, Line 514: const std::deque<std::shared_ptr<WriteOpState>> ops); > warning: parameter 'ops' is const-qualified in the function declaration; co Done -- To view, visit http://gerrit.cloudera.org:8080/18350 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I842476216f09150383d06c7a6f4c53bf7f2ec018 Gerrit-Change-Number: 18350 Gerrit-PatchSet: 2 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Fri, 25 Mar 2022 03:34:57 +0000 Gerrit-HasComments: Yes
