You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/05/22 09:39:37 UTC
[kudu-CR] threadpool-test: use test fixture
Hello Dan Burkert, David Ribeiro Alves, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6944
to review the following change.
Change subject: threadpool-test: use test fixture
......................................................................
threadpool-test: use test fixture
Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.h
2 files changed, 146 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/6944/1
--
To view, visit http://gerrit.cloudera.org:8080/6944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] threadpool-test: use test fixture
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6944
to look at the new patch set (#2).
Change subject: threadpool-test: use test fixture
......................................................................
threadpool-test: use test fixture
Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.h
2 files changed, 140 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/6944/2
--
To view, visit http://gerrit.cloudera.org:8080/6944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] threadpool-test: use test fixture
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool-test: use test fixture
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6944/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
Line 42: using strings::Substitute;
> warning: using decl 'Substitute' is unused [misc-unused-using-decls]
are these Tidy Bot warnings correct?
Line 53: ASSERT_OK(ThreadPoolBuilder(kDefaultPoolName).Build(&pool_));
looking at the cases, it seems like almost all of them end up "rebuilding". Maybe it would be cleaner to just not have any default pool and have every test be responsible for building whatever pool they want?
--
To view, visit http://gerrit.cloudera.org:8080/6944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] threadpool-test: use test fixture
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: threadpool-test: use test fixture
......................................................................
threadpool-test: use test fixture
Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Reviewed-on: http://gerrit.cloudera.org:8080/6944
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/util/threadpool-test.cc
M src/kudu/util/threadpool.h
2 files changed, 140 insertions(+), 149 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] threadpool-test: use test fixture
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: threadpool-test: use test fixture
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/6944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] threadpool-test: use test fixture
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: threadpool-test: use test fixture
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6944/1/src/kudu/util/threadpool-test.cc
File src/kudu/util/threadpool-test.cc:
Line 42: using strings::Substitute;
> are these Tidy Bot warnings correct?
Yeah; these declarations belong in the next patch, not this one. Will fix.
Line 53: ASSERT_OK(ThreadPoolBuilder(kDefaultPoolName).Build(&pool_));
> looking at the cases, it seems like almost all of them end up "rebuilding".
The token-based tests in the patches to follow generally use the default pool.
--
To view, visit http://gerrit.cloudera.org:8080/6944
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic76e5f946356d2cf6869cb7a665d0eeeeba5adde
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes