Alexey Serbin has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup ......................................................................
Patch Set 3: (9 comments) Just some nits for a while -- will take a deeper 'semantic' look shortly. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager-stress-test.cc File src/kudu/consensus/consensus_meta_manager-stress-test.cc: PS3, Line 18: #include <stdint.h> nit: prefer using <cstdint> I'm planning to update IWYU to give more straightforward suggestions. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.h File src/kudu/consensus/consensus_meta_manager.h: PS3, Line 59: ConsensusMetadataCreateMode create_mode What if this parameter was set to ConsensusMetadataCreateMode::FLUSH_ON_CREATE by default? That would allow for removal of some LOC in consensus_meta_manager-test.cc and other places where FLUS_ON_CREATE is the desired behavior and the cmeta_out is not needed. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/raft_consensus_quorum-test.cc File src/kudu/consensus/raft_consensus_quorum-test.cc: PS3, Line 191: scoped_refptr<ConsensusMetadata> cmeta; nit: drop this and leave the last parameter of the Create() call below by default. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/integration-tests/tombstoned_voting-itest.cc File src/kudu/integration-tests/tombstoned_voting-itest.cc: PS3, Line 18: #include <stdint.h> nit: consider replacing with <cstdint> and adding that into the list of the STD headers just below. I'm sorry for the inconvenience right now, but I'm planning to update IWYU-related stuff accordingly to make its suggestions more straightforward. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: PS3, Line 260: scoped_refptr<ConsensusMetadata> cmeta; nit: consider dropping this since it's not used in the scope and leaving the last parameter of the Create() call below by default. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_bootstrap-test.cc File src/kudu/tablet/tablet_bootstrap-test.cc: PS3, Line 146: scoped_refptr<ConsensusMetadata> cmeta; Nit: it seems this is not used, consider dropping it and leaving the last parameter for the Create() call below by default. Also, as mentioned elsewhere, consider having the second-from-the-tail parameter for Create() by default as well. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS3, Line 141: scoped_refptr<ConsensusMetadata> cmeta; Nit: consider dropping this since it's not used, leaving the last parameter for the Create() call below at its default value. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: PS3, Line 601: scoped_refptr<ConsensusMetadata> cmeta; Nit: cmeta is not used in this scope, right? Consider dropping it. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_source_session-test.cc File src/kudu/tserver/tablet_copy_source_session-test.cc: PS3, Line 157: scoped_refptr<ConsensusMetadata> cmeta; nit: consider dropping cmeta -- it's not used in this scope elsewhere but passing as the last parameter to the Create() call below, where it can be left by default. -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
