You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2018/02/11 23:01:01 UTC

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Michael Ho has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9282


Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_max_mem. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previou sreleases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_max_mem directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 20% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 228 insertions(+), 138 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.

Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3:

(2 comments)

> Patch Set 1:
> 
> (6 comments)
> 
> I went through the change and found that I would prefer to not access dependencies through the ExecEnv singleton, but instead inject them through the ctors and Init(). That way it feels easier to me to follow the code. I don't feel strongly about it though.

I'm curious how do others feel about this?

> 
> However, if we want to go with ExecEnv, we should probably remove some of the injected dependencies as well to keep it consistent.

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h@133
PS1, Line 133:   // Takes over ownership of the newly created 'service' which needs to have a lifetime
> The RpcMgr no longer owns the rpc service. RpcMgr::Shutdown() will call rpc
Shouldn't the RpcMgr own the services then?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc@83
PS1, Line 83:   mem_tracker_.reset(new MemTracker(-1, "Data Stream Manager Deferred RPCs",
> It seems the common pattern is to have the MemTracker inside the owning obj
That makes sense to me, thanks for the explanation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 03:23:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Lars Volker (Code Review)" <ge...@cloudera.org>.

Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(6 comments)

I went through the change and found that I would prefer to not access dependencies through the ExecEnv singleton, but instead inject them through the ctors and Init(). That way it feels easier to me to follow the code. I don't feel strongly about it though.

However, if we want to go with ExecEnv, we should probably remove some of the injected dependencies as well to keep it consistent.

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG@15
PS1, Line 15: previou
nit: typo


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h@133
PS1, Line 133:   // Takes over ownership of the newly created 'service'.
Can you explain why this method is needed? Could you just pass in a pointer here?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc@304
PS1, Line 304:     RETURN_IF_ERROR(data_svc_->Init(rpc_mgr_.get()));
The RpcMgr has a getter in the ExecEnv, too, so you could remove it from the ctor.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc@305
PS1, Line 305: data_svc_
this one, too


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc@83
PS1, Line 83:   mem_tracker_.reset(new MemTracker(-1, "Data Stream Manager Deferred RPCs",
I found this easier to follow when the MemTrackers were created in ExecEnv with the rest of the trackers. I don't feel strongly about it though.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc@46
PS1, Line 46: DataStreamService::DataStreamService(RpcMgr* rpc_mgr)
It seems odd to pass the rpc_mgr into both the ctor and Init().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Feb 2018 20:33:43 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h@252
PS4, Line 252: skip_gc = false
> I think it's going to be kind of hard for readers of the code and new calls
Yes, we can try to document it.

I guess the problem to me is that this function is unconditionally GC'ing when any MemTracker's limit is exceeded. However, IIUC, GC'ing should help if you exceeded the process mem limit. So, it's unclear to me if it's okay to add a check here to see if the mem_tracker exceeding the limit is the process_mem_tracker_ and only call GcMemory in that case.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 19:17:14 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Lars Volker, Tim Armstrong, Dan Hecht, 

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

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

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_max_mem. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_max_mem directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 20% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 282 insertions(+), 164 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Lars Volker, Tim Armstrong, Dan Hecht, 

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

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

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_mem_limit. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_mem_limit directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 20% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 283 insertions(+), 164 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Lars Volker, Tim Armstrong, Dan Hecht, 

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

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

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_mem_limit. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_mem_limit directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 5% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 282 insertions(+), 162 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/9282/5
-- 
To view, visit http://gerrit.cloudera.org:8080/9282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG@20
PS3, Line 20: 20
5%


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/service/data-stream-service.cc@41
PS3, Line 41: DEFINE_string(datastream_service_queue_mem_limit, "5%", queue_limit_msg.c_str());
5% seems ok to start with. Would be good to do some experiments to get an idea for what typical consumption is.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 07:21:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 5:

(I'm assuming that we'll validate that the mem limit works ok by doing additional concurrency testing before turning on KRPC by default).


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 01:38:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h@252
PS4, Line 252: skip_gc = false
> Yes, we can try to document it.
Should we change this so that we only GC if the process limit was exceeded? And then we don't need this flag, right?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 19:18:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9282/5/be/src/rpc/impala-service-pool.h@41
PS5, Line 41: service_queue_depth
service_queue_length?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 01:37:46 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3:

This should have read: 
> However, the dependencies are often *not* just between
 > the existence of the object but between the state of the bojects
 > (e.g. whether Init() was called yet), so it doesn't always help.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 03:44:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/exec-env.cc@304
PS2, Line 304:     RETURN_IF_ERROR(data_svc_->Init());
now that we don't pass parameters, if there are still dependencies in the ordering of these methods, how about noting that?


http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176
PS2, Line 176:   service_mem_tracker_->Release(transfer_size);
what is the impact of the transient double counting now?


http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/service/data-stream-service.h@45
PS2, Line 45:   /// Initializes the service by registering it with the singleton RPC manager.
note here that the RpcMgr::Init() must have already been called (or whatever the ordering dependency is).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2018 18:27:52 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h@133
PS1, Line 133:   // Takes over ownership of the newly created 'service'.
> Shouldn't the RpcMgr own the services then?
It used to be the case that ImpalaServicePool owns the service before this change. I intentionally factored that out and transferred the ownership to ExeEnv. DataStreamService is a global singleton. I found it useful to be able to access DataStreamService (via ExecEnv) for extended functionality (e.g. per thread cache of some objects) in the past during prototyping. Will update the comment in DataStreamService to indicate that it should be singleton.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 08:07:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG@20
PS1, Line 20: this flag is left at 0, in which case it will be set to 20% of process
> I was using the assumption of 32KB avg payload size, 500 nodes, 500 fragmen
It seems too aggressive for small-to-medium deployments for sure :). It might be a reasonable value for very large deployments with high concurrency, but I think that would require adjusting buffer_pool_limit too.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc@37
PS1, Line 37: DEFINE_int64(datastream_service_queue_mem_limit, 0,
It would be good to use ParseUtil::ParseMemSpec() for this for consistency and convenience - see what is done for --buffer_pool_limit. This lets users specify it as a bytes amount with the usual suffixes or a percentage.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Feb 2018 23:44:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG@20
PS1, Line 20: this flag is left at 0, in which case it will be set to 20% of process
> It seems too aggressive for small-to-medium deployments for sure :). It mig
Pushed it down to 5% for now. Until IMPALA-5859 is fixed, RPC retries may end up consuming quite a lot more network bandwidth.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.h@43
PS1, Line 43: MemTracker* mem_tracker
> it would be good to document these parameters. Is this the mem_tracker that
Done


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc@150
PS1, Line 150: or
> , otherwise
Done


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc@152
PS1, Line 152: std::
> nit: names.h should take care of that
Done


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr-test-base.h@133
PS1, Line 133:   // Takes over ownership of the newly created 'service'.
> Can you explain why this method is needed? Could you just pass in a pointer
The RpcMgr no longer owns the rpc service. RpcMgr::Shutdown() will call rpc service's Shutdown() function so the lifetime of the service should be as long as the rpc_mgr_. That's why the ownership is transferred to RpcMgrTestBase class instead. Comments added.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr.h@127
PS1, Line 127: mem_tracker
> document. and this is the service's mem tracker right? if so, maybe service
Done


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc@304
PS1, Line 304:     RETURN_IF_ERROR(data_svc_->Init(rpc_mgr_.get()));
> The RpcMgr has a getter in the ExecEnv, too, so you could remove it from th
Done


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/exec-env.cc@305
PS1, Line 305: data_svc_
> this one, too
This is a bit tricky as this is needed in data-stream-test.cc.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.h
File be/src/runtime/krpc-data-stream-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.h@234
PS1, Line 234: handed over to DataStreamService
> is that correct? Isn't incoming_request_tracker the DataStreamService's mem
DataStreamMgr / Exchange node to be more precise.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc@83
PS1, Line 83:   mem_tracker_.reset(new MemTracker(-1, "Data Stream Manager Deferred RPCs",
It seems the common pattern is to have the MemTracker inside the owning object (e.g. exec node, DataStreamRecvr etc) so having it externally in ExecEnv seems to be an uncommon pattern. The lifetime of the MemTracker of DataStreamMgr shouldn't be longer than that of the DataStreamMgr.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc@175
PS1, Line 175: Release
> I guess we were using the "Local" versions before since we were just transf
The "local" variant doesn't allow the tracker to have memory limit. There is a window but then both of these trackers have process_mem_tracker as their parents. I can change the order of Consume() and Release() to avoid under-counting if that's a big concern at all.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.h@58
PS1, Line 58:   MemTracker* mem_tracker() { return mem_tracker_.get(); }
> does that need to be public?
It's used in ExecEnv::Init().


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc
File be/src/service/data-stream-service.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc@37
PS1, Line 37: DEFINE_int64(datastream_service_queue_mem_limit, 0,
> It would be good to use ParseUtil::ParseMemSpec() for this for consistency 
Good idea. Done.


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.cc@46
PS1, Line 46: DataStreamService::DataStreamService(RpcMgr* rpc_mgr)
> It seems odd to pass the rpc_mgr into both the ctor and Init().
Good point. We should be able to access it through the singleton ExecEnv.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2018 08:24:51 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 6: Code-Review+2

Carry Tim's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 07:18:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 6:

Yes, we will do appropriate tuning for the queue memory limit to get reasonable performance before turning KRPC on by default.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 07:19:38 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h@252
PS4, Line 252: skip_gc = false
I think it's going to be kind of hard for readers of the code and new callsites to know why/when to set skip_gc=true.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:42:56 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG@20
PS1, Line 20: this flag is left at 0, in which case it will be set to 20% of process
> Devoting 20% of the process limit to this seems very high. The buffer pool 
I was using the assumption of 32KB avg payload size, 500 nodes, 500 fragments per host. Assuming each host has 64GB so process mem limit is 80% of it. The queue consumption translates to about 15% of process mem limit and I bumped it up to 20% just to be safe as broadcast exchange can have payload much larger than 32KB.

The consequence of having a value too low is that RPCs may have to be retried multiple times. That said, it sounds like 20% may be  too aggressive. Will trimming it down to 5% still too high or should we just hard code a value based on the assumption above ?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Feb 2018 23:03:57 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 6:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 07:19:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Lars Volker, Tim Armstrong, Dan Hecht, 

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

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

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_mem_limit. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_mem_limit directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 5% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 282 insertions(+), 162 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/9282/6
-- 
To view, visit http://gerrit.cloudera.org:8080/9282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/exec-env.cc
File be/src/runtime/exec-env.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/exec-env.cc@304
PS2, Line 304:     RETURN_IF_ERROR(data_svc_->Init());
> now that we don't pass parameters, if there are still dependencies in the o
Done


http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176
PS2, Line 176:   service_mem_tracker_->Release(transfer_size);
> what is the impact of the transient double counting now?
There may be a temporary window in which the process mem limit is exceeded, leading to other callers of MemTracker::TryConsume() to fail. Worst case, we may be double counting by # service threads * avg row batch size.

On the other hand, leaving the window open may lead to a higher chance of going over the process mem limit.


http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/service/data-stream-service.h@45
PS2, Line 45:   /// Initializes the service by registering it with the singleton RPC manager.
> note here that the RpcMgr::Init() must have already been called (or whateve
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Feb 2018 23:57:28 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 6: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 11:00:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Lars Volker, Tim Armstrong, Dan Hecht, Impala Public Jenkins, 

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

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

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_mem_limit. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_mem_limit directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 5% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 281 insertions(+), 161 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/9282/7
-- 
To view, visit http://gerrit.cloudera.org:8080/9282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/3//COMMIT_MSG@20
PS3, Line 20: 20
> 5%
Done


http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc@155
PS2, Line 155: CheckLimitExceeded
> Why don't we call AnyLimitExceeded()? If we're over the process limit it se
I thought about that option but I am concerned about calling Gc in the middle of the Rpc path. Updated the patch to make AnyLimitExceeded() to not GC if specified by caller.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc@159
PS3, Line 159:       c->DiscardTransfer();
> Maybe drop the lock at this point to avoid calling free() while holding the
Done


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@135
PS3, Line 135: TakeoverService
> nit: take over is two words, so TakeOverService
Done


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@234
PS3, Line 234:       dynamic_cast<PingServiceImpl*>(ping_impl)->mem_tracker()));
> dynamic_cast<> here and below shouldn't be necessary - don't need to check 
Done


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc@186
PS3, Line 186: dynamic_cast
> static_cast should also work here and below.
Done


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h@281
PS3, Line 281:   bool CheckLimitExceeded() const { return limit_ >= 0 && limit_ < consumption(); }
> Why not use the existing public LimitExceeded() or AnyLimitExceeded() APIs.
I am a bit concerned by the fact that exceeding the memory limit may lead to GcMemory being called in the middle of some critical path. May be an alternative would be to make LimitExceeded() take an argument to skip Gc and simply return true.

Implemented this idea in the new patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 08:46:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9282/1//COMMIT_MSG@20
PS1, Line 20: this flag is left at 0, in which case it will be set to 20% of process
Devoting 20% of the process limit to this seems very high. The buffer pool is given 80% of the process limit, so this leaves no guaranteed headroom for anything else. 

I.e. if the service expands up to its limit, it will squeeze out any non-buffer-pool memory from queries and likely cause query failures. It seems to somewhat defeat the purpose of limiting the amount of memory consumed if we're not limiting it low enough to prevent query failures.

I'd be interested in learning more about how the number was obtained and what the trade-offs are in setting it to a lower value



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Feb 2018 19:59:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/rpc/impala-service-pool.cc@155
PS2, Line 155: CheckLimitExceeded
Why don't we call AnyLimitExceeded()? If we're over the process limit it seems important to reject incoming RPCs.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/impala-service-pool.cc@159
PS3, Line 159:       c->DiscardTransfer();
Maybe drop the lock at this point to avoid calling free() while holding the lock? Might help in some cases where system is already overloaded.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h
File be/src/rpc/rpc-mgr-test-base.h:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@135
PS3, Line 135: TakeoverService
nit: take over is two words, so TakeOverService


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test-base.h@234
PS3, Line 234:       dynamic_cast<PingServiceImpl*>(ping_impl)->mem_tracker()));
dynamic_cast<> here and below shouldn't be necessary - don't need to check runtime type so static_cast should work.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc
File be/src/rpc/rpc-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/rpc/rpc-mgr-test.cc@186
PS3, Line 186: dynamic_cast
static_cast should also work here and below.


http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176
PS2, Line 176:   service_mem_tracker_->Release(transfer_size);
> The process limit is checked by looking at TC-malloc and the buffer pool, r
I think generally we should be conservative and call Consume() before Release(). I can see Michael's argument in this case we could cause unnecessary failures in that way. I think in either case the probability of problems is low so this seems ok.


http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/3/be/src/runtime/mem-tracker.h@281
PS3, Line 281:   bool CheckLimitExceeded() const { return limit_ >= 0 && limit_ < consumption(); }
Why not use the existing public LimitExceeded() or AnyLimitExceeded() APIs. It should also work for this purpose. I think having three similar public APIs is really confusing.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 06:31:55 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176
PS2, Line 176:   service_mem_tracker_->Release(transfer_size);
> There may be a temporary window in which the process mem limit is exceeded,
The process limit is checked by looking at TC-malloc and the buffer pool, rather than adding up the children consumption.  So, I don't think this impacts the process limit. 

Who are the parents of these two trackers?

Tim, what's your take on the order that the Consume() and Release() should occur in?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 00:06:27 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_mem_limit. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_mem_limit directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 5% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Reviewed-on: http://gerrit.cloudera.org:8080/9282
Reviewed-by: Michael Ho <kw...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 281 insertions(+), 161 deletions(-)

Approvals:
  Michael Ho: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 22:27:34 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

http://gerrit.cloudera.org:8080/#/c/9282/4/be/src/runtime/mem-tracker.h@252
PS4, Line 252: skip_gc = false
> Should we change this so that we only GC if the process limit was exceeded?
Actually, Tim pointed out that we only installed GC functions for process mem tracker so I was just misreading the code then. Sorry for the confusion. Will remove the flag.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 16 Feb 2018 21:50:16 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/5/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9282/5/be/src/rpc/impala-service-pool.h@41
PS5, Line 41: service_queue_depth
> service_queue_length?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 07:17:25 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3:

> (2 comments)
 > 
 > > Patch Set 1:
 > >
 > > (6 comments)
 > >
 > > I went through the change and found that I would prefer to not
 > access dependencies through the ExecEnv singleton, but instead
 > inject them through the ctors and Init(). That way it feels easier
 > to me to follow the code. I don't feel strongly about it though.
 > 
 > I'm curious how do others feel about this?
 > 

My feeling is that if something is a singleton, then it's usually easier for me to follow the code if it's accessed directly rather than adding indirection by passing around pointers to it (which ultimately I'll often need to trace back to figure out where the object came from anyway).  Additionally, at least in some places, the architecture dictates some of these daemon services are singletons, so I don't see the advantage in pretending that they parametrizable. 

On the other hand, passing by argument can help show dependencies between objects. However, the dependencies are often just between the existence of the object but between the state of the bojects (e.g. whether Init() was called yet), so it doesn't always help.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 03:43:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 3: Code-Review+1

I'm okay with this change modulo the choice of limit and ordering of memtracker updates. Tim, can you do the +2 once you guys agree on that and you're happy with the change? Lars, please see if you have any further comments.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 00:43:47 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 7: Code-Review+2

Fix a clang tidy issue.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 18:43:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 1:

(7 comments)

The high level discussion Tim brings up is important, but also prefetching some comments on the specifics of the implementation.

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.h
File be/src/rpc/impala-service-pool.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.h@43
PS1, Line 43: MemTracker* mem_tracker
it would be good to document these parameters. Is this the mem_tracker that the pool will use to account queued RPCs, or what?  Actually, I think it's the mem-tracker associated with 'service', right? How about calling it service_mem_tracker to make that clear?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc
File be/src/rpc/impala-service-pool.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc@150
PS1, Line 150: or
, otherwise


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/impala-service-pool.cc@152
PS1, Line 152: std::
nit: names.h should take care of that


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr.h
File be/src/rpc/rpc-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/rpc/rpc-mgr.h@127
PS1, Line 127: mem_tracker
document. and this is the service's mem tracker right? if so, maybe service_mem_tracker?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.h
File be/src/runtime/krpc-data-stream-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.h@234
PS1, Line 234: handed over to DataStreamService
is that correct? Isn't incoming_request_tracker the DataStreamService's mem_tracker?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/runtime/krpc-data-stream-mgr.cc@175
PS1, Line 175: Release
I guess we were using the "Local" versions before since we were just transferring the memory between siblings.  Why is it safe to now release it all the way and then consume (leaving a window of it being unaccounted)?


http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.h
File be/src/service/data-stream-service.h:

http://gerrit.cloudera.org:8080/#/c/9282/1/be/src/service/data-stream-service.h@58
PS1, Line 58:   MemTracker* mem_tracker() { return mem_tracker_.get(); }
does that need to be public?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Feb 2018 21:50:44 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/9282 )

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc
File be/src/runtime/krpc-data-stream-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9282/2/be/src/runtime/krpc-data-stream-mgr.cc@176
PS2, Line 176:   service_mem_tracker_->Release(transfer_size);
> The process limit is checked by looking at TC-malloc and the buffer pool, r
The parent trackers are both process memory tracker.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Feb 2018 00:36:33 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 17 Feb 2018 18:44:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.

Hello Lars Volker, Tim Armstrong, Dan Hecht, 

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

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

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

Change subject: IMPALA-6116: Bound memory usage of DataStreamSevice's service queue
......................................................................

IMPALA-6116: Bound memory usage of DataStreamSevice's service queue

The fix for IMPALA-6193 added a memory tracker for the memory consumed
by the payloads in the service queue of DataStreamService. This change
extends it by introducing a bound on the memory usage for that service
queue. In addition, it deprecates FLAGS_datastream_service_queue_depth
and replaces it with FLAGS_datastream_service_queue_mem_limit. These flags
only take effect when KRPC is in use and KRPC was never enabled in any
previous releases so it seems safe to do this flag replacement. The new
flag FLAGS_datastream_service_queue_mem_limit directly dictates the amount
of memory which can be consumed by the service queue of DataStreamService.
This allows a more direct control over the memory usage of the queue instead
of inferring via the number of entries in the queue. The default value of
this flag is left at 0, in which case it will be set to 5% of process
memory limit.

Testing done: exhaustive debug builds. Updated data-stream-test to
exercise the case in which the payload is larger than the limit.

Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
---
M be/src/rpc/impala-service-pool.cc
M be/src/rpc/impala-service-pool.h
M be/src/rpc/rpc-mgr-test-base.h
M be/src/rpc/rpc-mgr-test.cc
M be/src/rpc/rpc-mgr.cc
M be/src/rpc/rpc-mgr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/krpc-data-stream-mgr.cc
M be/src/runtime/krpc-data-stream-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/service/data-stream-service.cc
M be/src/service/data-stream-service.h
M tests/custom_cluster/test_krpc_mem_usage.py
15 files changed, 292 insertions(+), 170 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/82/9282/4
-- 
To view, visit http://gerrit.cloudera.org:8080/9282
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Idea4262dfb0e0aa8d58ff6ea6a8aaaa248e880b9
Gerrit-Change-Number: 9282
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>