Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/5241 )
Change subject: WIP KUDU-1563. Implement feature flag support IGNORE operations ...................................................................... Patch Set 9: (4 comments) http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/batcher.cc@275 PS9, Line 275: // The set of server features required by this rpc : std::unordered_set<uint32_t> required_server_features_; This is certainly future-proof for additional feature flags, but given that writes are a hot path, perhaps we'd be better served with a simple boolean for IGNORE_OPERATIONS? http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/batcher.cc@338 PS9, Line 338: required_server_features_.insert(tserver::TabletServerFeatures::IGNORE_OPERATIONS); Inserting to a hash-based set on every op is inefficient; it's a hash and comparison even when the set already contains the desired value. How about setting some local bool to true, then outside of the loop, doing just one insert to the set if the bool tests true? http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/client/client-test.cc@2488 PS9, Line 2488: TEST_F(ClientTest, TestRequiredServerFeatures) { Indentation is off, should be 2 char not 4 char. http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/5241/9/src/kudu/tserver/tablet_service.cc@164 PS9, Line 164: service Nit: prefix with tserver instead of service. -- To view, visit http://gerrit.cloudera.org:8080/5241 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I68133e9b92658e56152cdf00ba3f7f10b30e961b Gerrit-Change-Number: 5241 Gerrit-PatchSet: 9 Gerrit-Owner: Brock Noland <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 18 Feb 2020 01:34:07 +0000 Gerrit-HasComments: Yes
