Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15005 )
Change subject: clock: remove shared ownership ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/consensus/consensus_queue.h File src/kudu/consensus/consensus_queue.h: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/consensus/consensus_queue.h@572 PS3, Line 572: scoped_refptr<TimeManager> time_manager_; > Is it possible to get rid of shared ownership for this member as well? It Done in a separate patch: http://gerrit.cloudera.org:8080/15007 http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/integration-tests/ts_recovery-itest.cc File src/kudu/integration-tests/ts_recovery-itest.cc: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/integration-tests/ts_recovery-itest.cc@593 PS3, Line 593: ); > nit: drop parentheses? Done http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/server/server_base.h File src/kudu/server/server_base.h: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/server/server_base.h@104 PS3, Line 104: > nit: add const specifier? Done http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/server/server_base.cc@387 PS3, Line 387: ); > nit: drop the parentheses? Done http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/mvcc-test.cc@185 PS3, Line 185: ); > nit: drop the parentheses? Done http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/tablet.h@69 PS3, Line 69: > Nit: kind of weird placement? IWYU did this automatically. I'll move it. http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/tablet.h@75 PS3, Line 75: class LogAnchorRegistry; > Nit: while you're here, add a namespace comment? Done http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/tablet_bootstrap-test.cc File src/kudu/tablet/tablet_bootstrap-test.cc: http://gerrit.cloudera.org:8080/#/c/15005/3/src/kudu/tablet/tablet_bootstrap-test.cc@82 PS3, Line 82: using kudu::consensus::ConsensusMetadataManager; > Nit: maybe move this into the existing kudu namespace Done -- To view, visit http://gerrit.cloudera.org:8080/15005 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3e75a539c5de1c367d05784f544b96197249ec49 Gerrit-Change-Number: 15005 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 10 Jan 2020 19:46:05 +0000 Gerrit-HasComments: Yes
