[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8076 )

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.

This is a slightly cleaned up version of Henry's abandoned patch:
https://gerrit.cloudera.org/#/c/7673/

Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Reviewed-on: http://gerrit.cloudera.org:8080/8076
Reviewed-by: Sailesh Mukil 
Tested-by: Impala Public Jenkins
---
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, 128 insertions(+), 137 deletions(-)

Approvals:
  Sailesh Mukil: Looks good to me, approved
  Impala Public Jenkins: Verified

--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 7
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 6: Verified+1


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 03 Oct 2017 02:20:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 6:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1287/


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:16:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 6: Code-Review+2

Hit IMPALA-5999 twice in a row. Trying GVO again.

Rebase, carry +2.


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 6
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Mon, 02 Oct 2017 22:16:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1279/


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 22:20:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1279/


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:14:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 5: Code-Review+2

Hit a flaky test. Will investigate and open a JIRA.

Rebase, carry +2.


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 5
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:13:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 4: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/1276/


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 04:36:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1276/


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:33:36 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 4: Code-Review+2

Rebase, carry +2.


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 4
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:33:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 3: Code-Review+2


--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 29 Sep 2017 00:26:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8076 )

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.h@999
PS2, Line 999:   /// Init() were <= 0.
> add to comment:
Done


http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.cc@2078
PS2, Line 2078:   }
> i think we still need to reset these pointers since they hold a shared_ptr
Done



--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 26 Sep 2017 14:38:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Hello Dan Hecht,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/8076

to look at the new patch set (#3).

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.

This is a slightly cleaned up version of Henry's abandoned patch:
https://gerrit.cloudera.org/#/c/7673/

Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
---
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, 128 insertions(+), 137 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8076/3
--
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-Change-Number: 8076
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Dan Hecht (Code Review)]

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/8076/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

Line 2078:   }
i think we still need to reset these pointers since they hold a shared_ptr to 
'this'. Otherwise, we have a cycle and 'this' will never be destructed.

For that reason, I'm not really sold that moving the ownership of the 
ThriftServer's into this class is an improvement (due to this ownership cycle), 
but as long as you add the comment in the header file about that, we can go 
forward.


http://gerrit.cloudera.org:8080/#/c/8076/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:

Line 999:   /// Init() were <= 0.
add to comment:
Note that these hold a shared pointer to 'this', and so need to be reset() 
explicitly.


-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

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/8076/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1952: 
> what is the purpose of this? I think we should avoid shared_from_this() unl
Since the Thrift classes expect a boost::shared_ptr, we need to pass in a 
boost::shared_ptr to the interface of 
ImpalaHiveServer2ServiceProcessor, etc.

shared_from_this() is basically a shared_ptr version of the 'this' pointer. So 
this looks like the cleanest way to do this.


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 123: thrift_
> is that the thrift impala internal service port (as opposed to to krpc veri
It's the thrift internal service port. I renamed it to thrift_be_port.


Line 999:   /// Init() were <= 0.
> can you comment on who owns these? even better, if this class owns them, us
Changed to scoped_ptr.


PS1, Line 1002: ptr since krpc is on the way, how about thrift_be_server_? this goes away once 
Yes, that's right. I've renamed it.


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

PS1, Line 86: boost::shared_ptr impala_server
> why do we need shared ownership of this thing? who else needs to keep it al
Yes, they are a consequence of the Thrift interface expecting a 
boost::shared_ptr.

The Thrift classes are:
ImpalaServiceProcessor, ImpalaHiveServer2ServiceProcessor, etc.


-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil 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.

This is a slightly cleaned up version of Henry's abandoned patch:
https://gerrit.cloudera.org/#/c/7673/

Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
---
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, 123 insertions(+), 137 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8076/2
-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4786: Clean up how ImpalaServers are created
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS1, Line 1952: handler = shared_from_this();
what is the purpose of this? I think we should avoid shared_from_this() unless 
it's really the clearest way to do something. could you elaborate on what this 
is all about?


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impala-server.h
File be/src/service/impala-server.h:

PS1, Line 123: be_port
is that the thrift impala internal service port (as opposed to to krpc verison 
of that service), or something else?


Line 999:   /// Init() were <= 0.
can you comment on who owns these? even better, if this class owns them, use 
scoped_ptr to make that explicit (we just don't want to use shared_ptr, and 
shared ownership, generally unless it really makes sense. but scoped_ptr, which 
implies single ownership, is fine.


PS1, Line 1002: be_server_
since krpc is on the way, how about thrift_be_server_? this goes away once we 
fully switch ImpalaInternalService to krpc, is that right?


http://gerrit.cloudera.org:8080/#/c/8076/1/be/src/service/impalad-main.cc
File be/src/service/impalad-main.cc:

PS1, Line 86: boost::shared_ptr impala_server
why do we need shared ownership of this thing? who else needs to keep it alive 
beyond this function scope? (maybe it is needed, but let's reason through that).

Oh, maybe this (and the one in ImpalaServer shared_from_this) is a consequence 
of ImpalaServer being the thing that implements the thrift interface and thrift 
requires the shared_ptr?


-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8076

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.

This is a slightly cleaned up version of Henry's abandoned patch:
https://gerrit.cloudera.org/#/c/7673/

Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
---
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, 119 insertions(+), 136 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8076/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8076
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If388c5618258a9c4529cd1d63e956566b92bd0d8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Sailesh Mukil (Code Review)]

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 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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Dan Hecht (Code Review)]

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 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 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Henry Robinson (Code Review)]

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 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4786: Clean up how ImpalaServers are created

[Henry Robinson (Code Review)]

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