[kudu-CR] TabletReplica: Move Init() logic to Start()
Alexey Serbin has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7194/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 113: local_peer_pb_(std::move(local_peer_pb)), > It seems to be saying that protobuf doesn't generate a move constructor. I' SGTM -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
Mike Percy has submitted this change and it was merged. Change subject: TabletReplica: Move Init() logic to Start() .. TabletReplica: Move Init() logic to Start() This is a cleanup / refactor that will make it easier to implement tombstoned voting. In this patch we merge Init() and Start() since they aren't really logically different right now. An unrelated change in this patch is a minor API cleanup in TSTabletManager, where we now pass TabletReplica directly to TSTabletManager::OpenTablet() instead of registering it in a map first and then looking it up again later. Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Reviewed-on: http://gerrit.cloudera.org:8080/7194 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin--- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 166 insertions(+), 175 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] TabletReplica: Move Init() logic to Start()
Mike Percy has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7194/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 113: local_peer_pb_(std::move(local_peer_pb)), > I would expect LANG_CXX11 to be an internal macro of the compiler, so ideal It seems to be saying that protobuf doesn't generate a move constructor. I'll look into that soon but I'm going to commit this now. -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
Alexey Serbin has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] TabletReplica: Move Init() logic to Start()
Alexey Serbin has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7194/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 113: local_peer_pb_(std::move(local_peer_pb)), > Define it where? I'm not sure what you mean. Also, I find this error messag I would expect LANG_CXX11 to be an internal macro of the compiler, so ideally we should not be messing around this. But I haven't verified that -- may be it's not. In the latter case, we need to add -DLANG_CXX11 to the compiler/preprocessor flags. Yes, it's a bit surprising. -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
Mike Percy has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7194/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 113: local_peer_pb_(std::move(local_peer_pb)), > It seems we need to define LANG_CXX11 ? Define it where? I'm not sure what you mean. Also, I find this error message surprising. :) -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
Alexey Serbin has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 5: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/7194/5/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 113: local_peer_pb_(std::move(local_peer_pb)), > warning: passing result of std::move() as a const reference argument; no mo It seems we need to define LANG_CXX11 ? -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7194 to look at the new patch set (#5). Change subject: TabletReplica: Move Init() logic to Start() .. TabletReplica: Move Init() logic to Start() This is a cleanup / refactor that will make it easier to implement tombstoned voting. In this patch we merge Init() and Start() since they aren't really logically different right now. An unrelated change in this patch is a minor API cleanup in TSTabletManager, where we now pass TabletReplica directly to TSTabletManager::OpenTablet() instead of registering it in a map first and then looking it up again later. Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a --- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 166 insertions(+), 175 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7194/5 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] TabletReplica: Move Init() logic to Start()
Alexey Serbin has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] TabletReplica: Move Init() logic to Start()
Mike Percy has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 4: sadly, this is in dire need of a rebase -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] TabletReplica: Move Init() logic to Start()
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7194 to look at the new patch set (#4). Change subject: TabletReplica: Move Init() logic to Start() .. TabletReplica: Move Init() logic to Start() This is a cleanup / refactor that will make it easier to implement tombstoned voting. In this patch we merge Init() and Start() since they aren't really logically different right now. An unrelated change in this patch is a minor API cleanup in TSTabletManager, where we now pass TabletReplica directly to TSTabletManager::OpenTablet() instead of registering it in a map first and then looking it up again later. Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a --- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 151 insertions(+), 166 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7194/4 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] TabletReplica: Move Init() logic to Start()
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7194 to look at the new patch set (#3). Change subject: TabletReplica: Move Init() logic to Start() .. TabletReplica: Move Init() logic to Start() This is a cleanup / refactor that will make it easier to implement tombstoned voting. In this patch we merge Init() and Start() since they aren't really logically different right now. An unrelated change in this patch is a minor API cleanup in TSTabletManager, where we now pass TabletReplica directly to TSTabletManager::OpenTablet() instead of registering it in a map first and then looking it up again later. Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a --- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 139 insertions(+), 155 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7194/3 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] TabletReplica: Move Init() logic to Start()
Mike Percy has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: PS2, Line 334: scoped_refptr shared_metadata() const { return metadata_; } > do we really need this? you can create a scoped_refptr from a raw pointer r Yes but it's nice to consistently pass around the vehicle that makes it clear that TabletMetadata is refcounted. http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS2, Line 133: Peer > nit rename these to StartReplica* Done PS2, Line 148: Status StartPeer(const ConsensusBootstrapInfo& info) { > nit maybe rename the "regular" method to StartReplica() and this one to Sta Done http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: Line 84: Status Start(const consensus::ConsensusBootstrapInfo& info, > warning: function 'kudu::tablet::TabletReplica::Start' has a definition wit Done http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: PS2, Line 236: const scoped_refptr& deleter > remove this if not used? It's actually needed as a reference but not used directly. I'll add a comment. -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
David Ribeiro Alves has posted comments on this change. Change subject: TabletReplica: Move Init() logic to Start() .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: PS2, Line 334: scoped_refptr shared_metadata() const { return metadata_; } do we really need this? you can create a scoped_refptr from a raw pointer right? http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS2, Line 133: Peer nit rename these to StartReplica* PS2, Line 148: Status StartPeer(const ConsensusBootstrapInfo& info) { nit maybe rename the "regular" method to StartReplica() and this one to StartReplicaAndWaitUntilLeader http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: Line 84: Status Start(const consensus::ConsensusBootstrapInfo& info, > warning: function 'kudu::tablet::TabletReplica::Start' has a definition wit address this? http://gerrit.cloudera.org:8080/#/c/7194/2/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: PS2, Line 236: const scoped_refptr& deleter remove this if not used? -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] TabletReplica: Move Init() logic to Start()
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7194 to look at the new patch set (#2). Change subject: TabletReplica: Move Init() logic to Start() .. TabletReplica: Move Init() logic to Start() This is a cleanup / refactor that will make it easier to implement tombstoned voting. In this patch we merge Init() and Start() since they aren't really logically different right now. An unrelated change in this patch is a minor API cleanup in TSTabletManager, where we now pass TabletReplica directly to TSTabletManager::OpenTablet() instead of registering it in a map first and then looking it up again later. Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a --- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 133 insertions(+), 153 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7194/2 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] TabletReplica: Move Init() logic to Start()
Hello David Ribeiro Alves, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7194 to review the following change. Change subject: TabletReplica: Move Init() logic to Start() .. TabletReplica: Move Init() logic to Start() This is a cleanup / refactor that will make it easier to implement tombstoned voting. In this patch we merge Init() and Start() since they aren't really logically different right now. An unrelated change in this patch is a minor API cleanup in TSTabletManager, where we now pass TabletReplica directly to TSTabletManager::OpenTablet() instead of registering it in a map first and then looking it up again later. Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a --- M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 131 insertions(+), 152 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7194/1 -- To view, visit http://gerrit.cloudera.org:8080/7194 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib762db8cfaac628325feee445490713b5c555c5a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike PercyGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: David Ribeiro Alves