[kudu-CR] server: move apply pool into ServerBase
Hello Dan Burkert, David Ribeiro Alves, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/6984 to review the following change. Change subject: server: move apply_pool into ServerBase .. server: move apply_pool into ServerBase The server-wide apply_pool was instantiated in two places: SysCatalogTable (for the master) and TsTabletManager (for the tserver). This commit moves it into ServerBase and unifies the instantiation. Note: the apply pool semantics have not changed. Some interesting side effects: 1. The master will now generate apply pool metrics. 2. The apply pool is shut down a little bit later in server shutdown than it was before, though I don't see any issues with this. Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed --- M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/master/sys_catalog.h M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 48 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/6984/1 -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon
[kudu-CR] server: move apply pool into ServerBase
Hello Dan Burkert, David Ribeiro Alves, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6984 to look at the new patch set (#2). Change subject: server: move apply_pool into ServerBase .. server: move apply_pool into ServerBase The server-wide apply_pool was instantiated in two places: SysCatalogTable (for the master) and TsTabletManager (for the tserver). This commit moves it into ServerBase and unifies the instantiation. Note: the apply pool semantics have not changed. Some interesting side effects: 1. The master will now generate apply pool metrics. 2. The apply pool is shut down a little bit later in server shutdown than it was before, though I don't see any issues with this. Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed --- M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/master/sys_catalog.h M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 48 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/6984/2 -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] server: move apply pool into ServerBase
Adar Dembo has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 4: There were two interesting TSAN warnings here. 1. ksck_remote-test: this TSAN warning is about a data race in GetKuduKerberosPrincipalOidNid(), which is Kudu's certificate handling code. Unrelated to this patch. 2. tablet_replica-test: this one looks like it's related to the "consolidate Raft thread pool" patch. Need to think about it some more. -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] server: move apply pool into ServerBase
Alexey Serbin has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 4: Code-Review+2 This looks like a very nice clean-up which puts the apply_pool to the right place. -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] server: move apply pool into ServerBase
David Ribeiro Alves has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/6984/4/src/kudu/server/server_base.h File src/kudu/server/server_base.h: PS4, Line 108: apply_pool can you call this "tablet_apply_pool" instead so that it's clearer what it does? -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] server: move apply pool into ServerBase
Adar Dembo has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/6984/4/src/kudu/server/server_base.h File src/kudu/server/server_base.h: PS4, Line 108: apply_pool > can you call this "tablet_apply_pool" instead so that it's clearer what it Done -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] server: move apply pool into ServerBase
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6984 to look at the new patch set (#5). Change subject: server: move apply_pool into ServerBase .. server: move apply_pool into ServerBase The server-wide apply_pool was instantiated in two places: SysCatalogTable (for the master) and TsTabletManager (for the tserver). This commit moves it into ServerBase and unifies the instantiation. Note: the apply pool semantics have not changed. Some interesting side effects: 1. The master will now generate apply pool metrics. 2. The apply pool is shut down a little bit later in server shutdown than it was before, though I don't see any issues with this. Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed --- M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.cc M src/kudu/master/sys_catalog.h M src/kudu/server/server_base.cc M src/kudu/server/server_base.h M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 8 files changed, 48 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/6984/5 -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] server: move apply pool into ServerBase
David Ribeiro Alves has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] server: move apply pool into ServerBase
Todd Lipcon has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/6984/10//COMMIT_MSG Commit Message: PS10, Line 11: ServerBase per comment in the parent commit, I'm not wild about putting too much Kudu-specific stuff into server/. The idea was to keep it largely independent/reusable for any distributed system server type thing (eg if we add a long running canary role, or a REST/grpc proxy role, or separate load-balancer, etc, then they could reuse server/ as is). I know though that the master and tserver have grown quite a bit of duplicate stuff around 'Tablet hosting' functionality, and this is the latest in a long line of such duplications. As much as I hate deep class hierarchies, maybe we should introduce some intermediate class here like TabletHostingServer or somesuch? Or could we move ts_tablet_manager into a different directory and make it usable by the master for hosting the catalog table, and put this stuff there? -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] server: move apply pool into ServerBase
Adar Dembo has posted comments on this change. Change subject: server: move apply_pool into ServerBase .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/6984/10//COMMIT_MSG Commit Message: PS10, Line 11: ServerBase > per comment in the parent commit, I'm not wild about putting too much Kudu- I did a little bit of digging and I think we're a long way away from reusing TSTabletManager in the master. So I'll introduce an intermediate class instead. New patch soon. -- To view, visit http://gerrit.cloudera.org:8080/6984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7ffc886269aa6531517a52fef29c4408a925aed Gerrit-PatchSet: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
