You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/08/15 00:07:26 UTC
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7673
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
IMPALA-4786: Clean up how ImpalaServers are created
ImpalaServer had to be created via an awkward CreateImpalaServer()
method that took a lot of arguments. This patch refactors that code (in
anticipation of some KRPC changes) to follow a more normal lifecycle:
1. ImpalaServer* server = new ImpalaServer(ExecEnv*)
2. RETURN_IF_ERROR(server->Init()) // for error-returning init operations
3. RETURN_IF_ERROR(server->Start())
4. server->Join()
Also add ExecEnv::Init(), and move calls to ExecEnv::StartServices() to
ImpalaServer::StartServices(). This captures a dependency that KRPC will
rely on - where initialization of both ExecEnv and ImpalaServer need to
happen before services are started.
This sets up a clean-up of InProcessImpalaServer, which is too heavy for
the work that it does. That work is deferred to a follow-on patch.
Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
---
M be/src/exprs/expr-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/impalad-main.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/hdfs-util-test.cc
10 files changed, 108 insertions(+), 137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7673/1
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7673/1/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:
Line 96: RETURN_IF_ERROR(exec_env_->Init());
> I hit a bug in expr-test here where StartServices() starts up the statestor
Hm, I didn't see much of a rebase conflict here. I'm really hoping we can get rid of InProcessImpalaServer in short order, however.
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7673/1/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:
Line 96: RETURN_IF_ERROR(exec_env_->Init());
I hit a bug in expr-test here where StartServices() starts up the statestore subscriber but ImpalaServer tries to subscribe to topics below, which is invalid after it's started.
Not sure what that means for this patch but you'll see a rebase conflict for sure. It seems like the dependency between ExecEnv::Init() and other things is non-trivial.
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#2).
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
IMPALA-4786: Clean up how ImpalaServers are created
ImpalaServer had to be created via an awkward CreateImpalaServer()
method that took a lot of arguments. This patch refactors that code (in
anticipation of some KRPC changes) to follow a more normal lifecycle:
1. ImpalaServer* server = new ImpalaServer(ExecEnv*)
2. RETURN_IF_ERROR(server->Init()) // for error-returning init operations
3. RETURN_IF_ERROR(server->Start())
4. server->Join()
Also add ExecEnv::Init(), and move calls to ExecEnv::StartServices() to
ImpalaServer::StartServices(). This captures a dependency that KRPC will
rely on - where initialization of both ExecEnv and ImpalaServer need to
happen before services are started.
This sets up a clean-up of InProcessImpalaServer, which is too heavy for
the work that it does. That work is deferred to a follow-on patch.
Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
---
M be/src/exprs/expr-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/scheduling/scheduler.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/impalad-main.cc
M be/src/testutil/in-process-servers.cc
M be/src/testutil/in-process-servers.h
M be/src/util/hdfs-util-test.cc
10 files changed, 108 insertions(+), 137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/73/7673/2
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7673/1/be/src/testutil/in-process-servers.cc
File be/src/testutil/in-process-servers.cc:
Line 96: RETURN_IF_ERROR(exec_env_->Init());
> Hm, I didn't see much of a rebase conflict here. I'm really hoping we can g
Yeah I reverted the change - changing the startup order caused other problems.
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has abandoned this change.
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
Abandoned
This patch is continued in:
https://gerrit.cloudera.org/#/c/8076/
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7673/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 212: LOG(INFO) << "Starting global services";
that should probably be updated
http://gerrit.cloudera.org:8080/#/c/7673/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:
Line 1006: boost::shared_ptr<ThriftServer> be_server_;
why do we need shared ptrs for these? can we not have single ownership?
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-4786: Clean up how ImpalaServers are created
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/7673/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:
PS2, Line 212: LOG(INFO) << "Starting global services";
> that should probably be updated
Done
http://gerrit.cloudera.org:8080/#/c/7673/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS2, Line 2035:
:
Added these logs back.
PS2, Line 2021: LOG(INFO) << "Impala HiveServer2 Service listening on " << hs2_port;
Spurious log.
Line 2046: shutdown_promise_.Get();
We need to join the be_server_, hs2_server_ and beeswax_server_ too here. I'll add it in the next patchset.
http://gerrit.cloudera.org:8080/#/c/7673/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:
Line 1006: boost::shared_ptr<ThriftServer> be_server_;
> why do we need shared ptrs for these? can we not have single ownership?
I looked through the code and the usage of these pointers and I agree that we can leave them as raw pointers due to single ownership. Done.
--
To view, visit http://gerrit.cloudera.org:8080/7673
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d55fbe0f4f7a1fd48993da46863b66e521feaae
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes