[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Perf:
Ran the full set of workloads (TPC-H, TPC-DS, targeted) on a 16 node
cluster. Everything was within normal variance.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Reviewed-on: http://gerrit.cloudera.org:8080/7408
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 289 insertions(+), 342 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 9: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 9:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..

IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Perf:
Ran the full set of workloads (TPC-H, TPC-DS, targeted) on a 16 node
cluster. Everything was within normal variance.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 289 insertions(+), 342 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7408/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 9: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 8: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7408/7/be/src/runtime/disk-io-mgr-scan-range.cc
File be/src/runtime/disk-io-mgr-scan-range.cc:

Line 47: DCHECK(reader_lock.mutex() == _->lock_ && 
reader_lock.owns_lock());
> does that belong in the scope? seems clearer to start the scope with the lo
Done


http://gerrit.cloudera.org:8080/#/c/7408/7/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:

PS7, Line 801: be an I/O
> is that suppose to say buffer?  Or does it mean I/O operation?
Done. It's an I/O request, i.e. RequestRange.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Hello Michael Ho, Dan Hecht,

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

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

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

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..

IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Perf:
Ran the full set of workloads (TPC-H, TPC-DS, targeted) on a 16 node
cluster. Everything was within normal variance.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 289 insertions(+), 342 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/7408/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7408
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 7: Code-Review+1

Carry +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 7:

I thought about it some more and I'm not sure that adding meaningful timers is 
simple. We already have TotalStorageWaitTime that measures the time the client 
spent blocked for a fragment. It might make sense to add a timer per exec node 
called "IoWaitTime" or "StorageWaitTime" that gives the granular timing.

It's harder to produce a meaningful time for how long the queue was full, i.e. 
we were blocked on compute, because there could be many scan ranges per thread. 
We could include a counter of the number of times the queue was filled up, e.g. 
"IoBufferQueueFullCount", but that might not be terribly intuitive.

I think that would give us enough information to see if the queue was too small 
- in that case we'd see significant IoWaitTime and non-zero 
IoBufferQueueFullCount

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 7:

I did a pass and think it makes sense. Took me a while just to page the code 
back in, hadn't looked at the DiskIoMgr in a while. I appreciate the 
simplification here :) I'd like to look again when I'm fresh later this week, 
since it sounds like this isn't holding anything up.

Would you mind adding a timer to measure how much time is spent waiting on the 
consumer?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7408/6/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

PS6, Line 373:  ObjectPool pool;
> actually, is it not used ?
Done


PS6, Line 426: pool_.Clear();
> pool_ not used in this loop ?
Its used indirectly via InitRange()->AllocateScanRange(). Kind of confusing.

Added comments to explain the intent. I also realised there are some places 
where it's unnecessary - it's not actually a loop.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Hello Michael Ho,

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

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

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

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..

IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Perf:
Ran the full set of workloads (TPC-H, TPC-DS, targeted) on a 16 node
cluster. Everything was within normal variance.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 288 insertions(+), 342 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7408/6/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

PS6, Line 373:  ObjectPool pool;
actually, is it not used ?


PS6, Line 426: pool_.Clear();
pool_ not used in this loop ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 6: Code-Review+1

Carry +1 from kwho

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7408/4/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

PS4, Line 375: pool_.reset(new ObjectPool);
> Not your change but can't this just be allocated on the stack ?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Hello Michael Ho,

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

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

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

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..

IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Perf:
Ran the full set of workloads (TPC-H, TPC-DS, targeted) on a 16 node
cluster. Everything was within normal variance.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 290 insertions(+), 335 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 4: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7408/4/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

PS4, Line 375: pool_.reset(new ObjectPool);
Not your change but can't this just be allocated on the stack ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 3:

Updated commit message with Perf. I did run benchmarks on a 16 node and it 
looked good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#4).

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..

IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Perf:
Ran the full set of workloads (TPC-H, TPC-DS, targeted) on a 16 node
cluster. Everything was within normal variance.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 266 insertions(+), 309 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..


Patch Set 3:

Did you run some perf tests to make sure that there is no regression ?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5417: make I/O buffer queue fixed-size

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#2).

Change subject: IMPALA-5417: make I/O buffer queue fixed-size
..

IMPALA-5417: make I/O buffer queue fixed-size

This removes the dynamically-varying queue size behaviour in the I/O
manager. The motivation is to bound resource consumption of scans
and make it possible to reserve memory for I/O buffers upfront.

Does some cleanup/documentation of the locking policy. Fix some cases
in ScanRange::GetNext() where members documented as being protected by
ScanRange::lock_ were accessed without holding it. I think the races
were either benign or prevented by holding DiskIoRequestContext::lock_
in practice.

Testing:
Ran exhaustive build.

Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
---
M be/src/runtime/disk-io-mgr-internal.h
M be/src/runtime/disk-io-mgr-reader-context.cc
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
6 files changed, 266 insertions(+), 309 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If7cc3f7199f5320db00b7face97a96cdadb6f83f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong