You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Yida Wu (Code Review)" <ge...@cloudera.org> on 2021/10/26 21:18:30 UTC
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Yida Wu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17979
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly, but in order to guarantee a page not
being cut into two parts in different blocks, the block size
could be a little different to each other in practice. The default
block size is the minimum value between 1/4 default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 5% of the total, because right now the system will
reserve 20% memory as unused memory, using 5% for the emergency
case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 5% of the total memory.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
11 files changed, 1,075 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/1
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
memory.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
12 files changed, 1,225 insertions(+), 150 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/6
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9669/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Oct 2021 21:29:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 17: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8491/
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 17
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Aug 2022 00:43:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 23: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 23
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 28 Sep 2022 03:50:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#14). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
system memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
17 files changed, 1,571 insertions(+), 159 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/14
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 14
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 23: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 23
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 28 Sep 2022 09:02:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 15: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 15
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Aug 2022 16:42:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 20:
(9 comments)
Looks great!
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG@32
PS20, Line 32: If the system decides to fetch a block, the block will be
: stored in the memory until all of the pages in the block are read
: or the query ends.
nit. suggest to present the positive case first. That is, moving it before "if not, we will go" at line 29.
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG@45
PS20, Line 45: the read buffer will be disabled.
nit. May mention that the memory borrowed from the reserved memory will be returned immediately after use.
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc@2183
PS20, Line 2183: // Check the metrics that we use the read buffer for reading.
nit. 'we use' -> 'we did use'.
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc@2200
PS20, Line 2200: f (buffer_size == small_size)
To improve the readability, it looks like various IF tests inside this block can be avoided if a helper function testCapacityForReadBuffer(int64_t buffer_size) can be written that tests one buffer size only.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1070
PS13, Line 1070:
nit exists
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1078
PS13, Line 1078: [read_buffer_block, tmp_file = this](const Status& fetch_status) {
nit. Can this line be moved above 1074? That is, if remote batch reading is disabled, return null immediately. No need to do the work from 1074 to 1077.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1104
PS13, Line 1104: ile* TmpFileRemote::GetR
Looks like this function does more than what the function name implies. You may need to add a comment at least. Is there a strong need to do this check?
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/mem-info.cc
File be/src/util/mem-info.cc:
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/mem-info.cc@265
PS20, Line 265: (process_avail_mem != nullptr)
may just say
if (process_avail_mem) *process_avail_mem = avail_mem;
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/metrics.h@272
PS20, Line 272: /// The hwm value is also updated atomically.
May add "Also return the updated current value." here.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 20
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 14:21:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@195
PS1, Line 195:
> Can we use MemBlockState state_ here?
Because the naming in DiskFile is "file_status_", maybe just keep them the same, otherwise it may be good to change all of them, including the interface names, but should be some work.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-context.cc
File be/src/runtime/io/request-context.cc:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-context.cc@201
PS3, Line 201: unstarted_remote_file_op_ranges_;
> Maybe named to unstarted_remote_file_op_ranges_?
Done
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h
File be/src/runtime/io/request-ranges.h:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h@118
PS3, Line 118: WRITE,
> May need to explain what it is.
Done
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h@702
PS3, Line 702:
> nit. upload the file to a remote location.
Done
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h@708
PS3, Line 708:
> nit. the fetch file operation from a remote site.
Done
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/scan-range.cc@171
PS3, Line 171: the range
: // is supposed to be read in one round.
> Suggest to remove as there is no guarantee.
Changed to "supposed".
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/scan-range.cc@178
PS3, Line 178: read_status
> need to check the status.
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@257
PS2, Line 257: Status setup_read_buffer_status = SetUpReadBufferParams();
: if (!setup_read_buffer_status.ok()) {
> If handling the rare case is simple task, I feel we should do so.
Changed. If the file size is smaller than the max block size, set the block size as file size. Otherwise block size is the max block size, which is 16MB.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@1039
PS2, Line 1039: read_buffer_block->NotifyAllWaits();
> In practice, the read buffer memory is always full during the big queries (
Have a simple test today (15x tpcds, q67, c5d.4xlarge 16u32g, 1G read buffer).
1. Disabled is set: (Time: 135s) (Data Read: 13.8GB)
2. Disabled is not set: (Time: 150s) (Data Read: 17.7GB)
As expected, if the disabled is not set, performance is worse because more data is read (more duplicated read). It could be a little different for other queries, but if the read buffer is not available (full) for most of the time, which is quite likely when spilling large amount of data, disabling the file from batching read when failing to reserve space could be a better solution.
I think the next optimization is to make the read buffer more available, maybe using a better eviction policy.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Nov 2021 01:05:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 5:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@396
PS5, Line 396: DCHECK(disk_file_dst_ != nullptr);
: DCHECK(disk_file_src_ != nullptr);
May need to add a comment to describe these two files. An example would be helpful too.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: "Mem block '$0' has been deleted, path: '$1'", buffer_idx, remote_file_path));
Why can't we use this DISABLED memory block here? Looks like we should not miss the opportunity of a good use of the memory block.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@436
PS5, Line 436: ScopedHistogramTimer read_timer(queue->read_latency());
This is not used.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@445
PS5, Line 445: status = Status(TErrorCode::DISK_IO_ERROR, GetBackendString(),
: GetHdfsErrorMsg("Error reading from HDFS file: ", remote_file_path));
Should we set the memory block allocated above to DISABLED here so that it can be used by reading from other offsets?
Also, I think we should close the file even in the error reading case.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Nov 2021 17:39:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9746/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 17:37:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
13 files changed, 1,304 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/8
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly, but in order to guarantee a page not
being cut into two parts in different blocks, the block size
could be a little different to each other in practice. The default
block size is the minimum value between 1/4 default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 5% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
5% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 5% of the total memory.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
12 files changed, 1,110 insertions(+), 151 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/4
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9719/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 03 Nov 2021 20:44:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly, but in order to guarantee a page not
being cut into two parts in different blocks, the block size
could be a little different to each other in practice. The default
block size is the minimum value between 1/4 default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 5% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
5% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 5% of the total memory.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
11 files changed, 1,098 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/3
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: read_buffer_bloc, MemBlockStatus::DISABLED, dstl, &read_buffer_lock)) {
> Agree that the memory for MemorBlock structures is not the concern.
I think the mechanism is like this, any read(pin) who wants the block memory, it will apply.
In this case, if the memory block is set to DISABLED, that means the memory was returned before here, the one (probably not this query, another thread) who wants the block memory can get the reservation. I guess it is how the memory of a block is reused.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@430
PS5, Line 430: RN_IF_ERROR(
> Thanks a lot for trying it out!
Tried a way like closing the file handle if two blocks have used. The performance can drop around 10% if we go exclusively for the file handle(each time one block by one block). If increasing the blocks number being read before closing the file handle, it will hang in hdfs interface. I think the hanging is something scary, may need some time to figure out the root cause before adding it
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
PS6, Line 411: tmp_dirs_remote_ctrl_.num_read_buffer_blocks_per_file_ =
: static_cast<int>(tmp_dirs_remote_ctrl_.remote_tmp_file_size_
: / tmp_dirs_remote_ctrl_.read_buffer_block_size_);
> Can you explain what happens when the system limit is reached and we can't
We will go "read by page" instead in this case. Because the "read by page" doesn't require extra memory, and the only difference is that it is not batching reading, however, compared to waiting for the memory for a block, an instant read and reply results in better performance in practice.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 23:07:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#20). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch read from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of the read buffer is where to get the extra memory
for it, because when impala starts to spill data, it means the
process lacks of memory to use. By default, impala process will
reserve 20% of the total system memory as unused memory, and here
we will use this unused memory for the read buffer because it is
reasonable to use it for the emergency case like spilling.
The maximum bytes of the read buffer memory are no more than 10%
of the total system memory and 50% of the unused memory. Also,
if the unused memory is less than 5% of the total system memory,
the read buffer will be disabled.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number is also restricted
by the process memory limit, which can not exceed 10% of the process
memory limit.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/mem-info.cc
M be/src/util/mem-info.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
19 files changed, 1,648 insertions(+), 156 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/20
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 20
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 20:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11337/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 20
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 12 Sep 2022 17:12:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10889/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 13
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 28 Jun 2022 00:27:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 12:
(19 comments)
A mix of questions and suggestions. I didn't see anything clearly wrong.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@55
PS12, Line 55: class MemBlock {
This seems like a fairly leaky abstraction. It mostly exists to handle the lifecycle around data_, but also relies on external operations to do lots of the actual management. Not sure I have specific improvements to suggest however.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@60
PS12, Line 60: DCHECK_EQ(static_cast<int>(status_), static_cast<int>(MemBlockStatus::DISABLED));
Can we also verify no memory leaks by checking `data_`?
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@220
PS12, Line 220: std::unique_ptr<int64_t[]> page_cnts_per_block_;
This seems a little funny, but I think I see why it's necessary. A const vector wouldn't let you update element values, and a non-const vector could potentially be resized. This is the only way to get a runtime fixed-size array.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@386
PS12, Line 386: bool DoReadFromReadBuffer(
This function doesn't take any action, so I'm not really sure why it's called "DoRead". I'd opt for Can, Should, or Must (depending on expectations here).
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@446
PS12, Line 446: // Helper function to check the status of a read buffe block.
nit: buffe -> buffer
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@458
PS12, Line 458: // Helper function to delete the read buffe block.
nit: buffe -> buffer
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@516
PS12, Line 516: /// The lock also protects the memory blocks from destruction, if the disk file has.
"if the disk file has" seems like an incomplete sentence.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.cc@99
PS12, Line 99: void MemBlock::Delete(bool* reserved, bool* allocated) {
Some unit tests around MemBlock transitions might be useful.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.cc@116
PS12, Line 116: SetStatusLocked(lock, MemBlockStatus::DISABLED);
It would be nice to have a DCHECK(data_ == nullptr) here. It would be true immediately after freeing ALLOC memory, or in other states (because no memory should have been allocated).
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-io-mgr.cc@463
PS12, Line 463: // If there was an error during reading, keep the old status.
Might be helpful to log errors that we can't return.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/request-ranges.h
File be/src/runtime/io/request-ranges.h:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/request-ranges.h@152
PS12, Line 152: RequestRange(RequestType::type request_type, int disk_id = -1, int64_t offset = -1)
How does an offset of -1 differ from 0?
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc@156
PS12, Line 156: if (!use_mem_buffer) {
Since we asserted above that use_local_buff implies !use_mem_buffer, this check is redundant.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc@291
PS12, Line 291: // 1. If it is the local buffer file is not deleted(evicted) yet.
Change to "If the local buffer file ..."
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr-test.cc@1835
PS12, Line 1835: int64_t file_size = 512 * 1024 * 1024;
Why was this changed?
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc@108
PS12, Line 108: "files. Only valid when the remote_batch_read is true.");
"Only valid when remote_batch_read is true." seems more consistent with how we talk about other flags.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc@117
PS12, Line 117: "Set if the system uses batch reading for the remote temporary files.");
This description could be more descriptive. Maybe "Set to prefetch additional blocks when reading from remote temporary files." Unless prefetch is the wrong term; it's not clear what's being batched without digging into the source code a lot.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc@259
PS12, Line 259: LOG(WARNING) << "Disable the read buffer for the remote temporary files "
"Disabled the read buffer for remote temporary files due to errors in read buffer parameters: "
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc@1116
PS12, Line 1116: // ahead of the real allocation, we need to decrease it if the block is reversed
reversed -> reserved
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc@1132
PS12, Line 1132: // Set a flag to notify other threads which are hold the file lock to release,
nit: hold -> holding
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 12
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 20 Jun 2022 21:43:22 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 15:
Fixed a testcase failure in TSAN build, could be related to an old issue IMPALA-4249.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 15
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 23:08:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 22: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8599/
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 22
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Sep 2022 21:21:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 22: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 22
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Sep 2022 16:10:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 16:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/11213/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 16
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Aug 2022 21:24:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 17:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8491/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 17
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Aug 2022 22:03:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 21:
(8 comments)
Thanks Qifan for the review.
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG@32
PS20, Line 32: and disable this block, because it may be good to avoid
: duplicated reads from the remote fs for the same content.
:
> nit. suggest to present the positive case first. That is, moving it before
Done
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc@2183
PS20, Line 2183: WaitForDiskFileStatus(file1->DiskFile(), DiskFileStatus::PERSISTED);
> nit. 'we use' -> 'we did use'.
Done
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/runtime/tmp-file-mgr-test.cc@2200
PS20, Line 2200: SSERT_OK(file_group.Read(handl
> To improve the readability, it looks like various IF tests inside this bloc
Added a helper function SetReadBufferSizeHelper().
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1070
PS13, Line 1070:
> nit exists
Done
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1078
PS13, Line 1078: [read_buffer_block, tmp_file = this](const Status& fetch_status) {
> nit. Can this line be moved above 1074? That is, if remote batch reading is
The function firstly check whether the local buffer exists (even batch reading is disabled, we could still have the disk buffer), if not, and batch reading enabled, we will try to see if there is an available buffer in memory. So we are not able to move the line ahead.
Also, this function is changed a lot in part 2, it may be better to keep optimizing in part 2 if there is no safety issue here.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1104
PS13, Line 1104: ile* TmpFileRemote::GetR
> Looks like this function does more than what the function name implies. You
Added a comment in the tmp-file-mgr.cc. Already a comment in the header of IncrementReadPageCount() in the tmp-file-mgr-internal.h. I think it is more efficient, because otherwise we need to fetch the lock to double check whether all the pages are read every time after the increment. Also it ensures that only one thread knows the "all_read" status and does the buffer removing job.
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/mem-info.cc
File be/src/util/mem-info.cc:
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/mem-info.cc@265
PS20, Line 265: (process_avail_mem) *process_a
> may just say
Done
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/metrics.h
File be/src/util/metrics.h:
http://gerrit.cloudera.org:8080/#/c/17979/20/be/src/util/metrics.h@272
PS20, Line 272: /// The hwm value is also updated atomically.
> May add "Also return the updated current value." here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 17:57:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 8: Code-Review+1
+1 as Abhishek may take a look.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 08 Dec 2021 14:45:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 6:
(18 comments)
Thanks Qifan for the detailed review.
Changed the name to "read buffer block", previously was "read buffer" or "mem block", and MemBlock is the implementation of the "read buffer block".
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@52
PS5, Line 52:
> Need a description for MemBlock class.
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@62
PS5, Line 62:
> nit. Return whether the memory is reserved or allocated before deletion to
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@68
PS5, Line 68:
> nit. may switch the order of the two arguments as the lock appears first in
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@168
PS5, Line 168: /// The status of the memory block.
> nit. May describe the content of a memory block a little bit. For example,
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@200
PS5, Line 200: DiskFile(const std::string& path, DiskIoMgr* io_mgr, int64_t file_size,
> nit missing the comment.
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@206
PS5, Line 206:
> Since this class is nested within DiskFile class, suggest to call it ReadBu
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@206
PS5, Line 206:
: virtual ~DiskFile() {}
> A little bit confusing on how these two names relate to mem blocks. See my
Previously, a read buffer and a memory block are the same thing in our patch. Changed to name it as "read buffer block", because compared to "memory block", the TmpFileMgr may be easier to know what the block is used for by its name. And the block may not be necessarily in memory, at first we think to have it in disk, the name could be easier to extend if later we have some different implementation.
Added some comments, hope to make it clearer.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@210
PS5, Line 210: page, each time
> Suggest to use a name relate to memory block, say mem_block_size_?
Changed to read_buffer_block_size_.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@213
PS5, Line 213: ilable or not.
> suggest to rename to num_mem_blocks_.
Changed to num_of_read_buffer_blocks_.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@326
PS5, Line 326: id UpdateReadBufferMetaData(int64_t offset) {
> I saw the IF check below which should take care of index being 12. Since th
It depends on the file size.
For example, if the file size is 128MB, block size is 8MB. Then there will be 128/8 = 16 mem blocks per file.
Therefore, there is no limitation that to be only 4 blocks per file, the number of blocks is decided by default file size and default block size. In this case, the last index is 15 for offset over 120MB.
There could be a case that the actual file size is over the default one a little bit, but it is assumed no more than a page size. For example, it could be 129MB for the file, therefore, the actual size of the last block can be 9MB, but it is not a huge difference compared to other blocks.
The number of read blocks is calculated here:
https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.cc@116
PS5, Line 116:
> Since the memory is actually deleted in this method, maybe we should call t
Think for a while, maybe DISABLED is better, because it means the block doesn't allow to use anymore. DELETED may mean sth has been deleted, but in our case, it doesn't necessarily delete anything for being the state.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@396
PS5, Line 396: // Fetch the data from the source file (remote) to the destination file (local).
: DCHECK(disk_file_dst_ != nullptr);
> May need to add a comment to describe these two files. An example would be
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: read_buffer_bloc, MemBlockStatus::DISABLED, dstl, &read_buffer_lock)) {
> Why can't we use this DISABLED memory block here? Looks like we should not
Because the memory of the block is released, if this case happens, we can't use the memory block unless we can reserve the memory again. But we don't allow the block status to go back from DISABLED to RESERVED, and the case when DISABLED happens, it could be that the query is finished or cancelled, or all the pages of the block are read, all cases should have no need to fetch anything anymore.
Added some comments.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@436
PS5, Line 436: atus = Status(TErrorCode::DISK_IO_ERROR, GetBackendStri
> This is not used.
The read timer works when it is destructed. It counts the time interval during its life cycle.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@445
PS5, Line 445: queue->read_size()->Update(local_file_size);
: disk_file_dst_->SetReadBufferBlockStatus(
> Should we set the memory block allocated above to DISABLED here so that it
Do you mean delete the memory block if fails? Right now it is handled by the callback function in TmpFile, because the deletion of a block needs to add some number to the TmpFileMgr metrics according to different block status, and the DiskIoMgr isn't aware of TmpFileMgr's structure.
So I guess the first question is the same that the callback function would set the block to DISABLED if fails.
The failure handle is here:
https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@1040
Added some comments.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h@147
PS5, Line 147: read_buffer_block_size_;
> nit. This name sounds like the buffer is remote. Maybe renamed as mem_block
Changed to read_buffer_block_size_.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h@151
PS5, Line 151: num_read_buffer_blocks_per_file_;
> similar comment as above. num_mem_blocks_per_file instead?
Changed to num_read_buffer_blocks_per_file_.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h@158
PS5, Line 158: max_read_buffer_size_;
> max_mem_block_size_?
Changed to max_read_buffer_size_.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Nov 2021 17:44:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 4:
(4 comments)
Looks great.
I have only the following two remaining concerns.
1. Whether the memory block lock can be integrated better with the disk file lock;
2. Once a memory block is disabled and its memory is returned to system, can it be put into use again? Let us say there are two HJs and both spill. Assume a memory block B can serve both overspills. Ideally B can serve HJ1 first followed by HJ2 or vice versa.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@70
PS1, Line 70: DCHECK_EQ(static_cast<int>(status_), static_cast<int>(MemBlockStatus::RESERVED));
: // Use malloc, could be better to alloc from a buffer pool.
: data_ = static_cast<uint8_t*>(malloc(size));
: DCHECK(data_ != nullptr);
: SetStatusLocked(lock, MemBlockStatus::ALLOC);
: }
:
: uint8_t* data() { return data_; }
:
> Yeah, maybe fewer functions could be better.
Sounds good.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@193
PS1, Line 193:
> I think the problem is even we use shared_mutex for the disk file lock, we
Some design may be needed in this area.
For example, if the disk file level lock is acquired for writing, then there is no point to get the memory block lock.
If the disk file level lock is not acquired (say due to read case), then the memory block lock is needed.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@195
PS1, Line 195:
> Because the naming in DiskFile is "file_status_", maybe just keep them the
Okay.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@1039
PS2, Line 1039: read_buffer_block->NotifyAllWaits();
> Have a simple test today (15x tpcds, q67, c5d.4xlarge 16u32g, 1G read buffe
Good test!
Once we disable the memory block here, when does it get used again?
My original concern is whether it can be used again hopefully (i.e. when the memory threshold test on dofetch becomes true again).
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Nov 2021 20:19:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 9:
Mainly optimizing the comments.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 9
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 10 Jan 2022 23:20:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 5:
(4 comments)
Based on Riza's performance test on patch 7, there could be a 10-16% improvement for small queries(Q64/65), but for big queries(Q4/78), could have 11-13% regression.
https://docs.google.com/spreadsheets/d/1GtIbB7Unc58nxxsvvpf3UhJfD6uBk1t37AeFQXaGAT0/edit#gid=588848535
The result shows that the batching read could increse the bandwidth usage and then improve the performance. However for queries involving large amount of spilling, the buffer is not sufficiently used because the reads on the spilled data are quite random, and therefore the in-memory blocks are hard to release and circulate. After a deeper investigation, the main cause is found related to the writing of the tmp files. All the fragments are mixed in one file or block, therefore the reading behavior is quite random even when the query is doing sequential scanning. Will file another jira to optimize because it needs quite a lot structure changes on the tmp file writing.
This patch added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/ScratchBytesReadUseLocalDisk. Thanks Riza for contributing the code. Also added reusing hdfs file handle while fetching the buffer blocks.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@430
PS5, Line 430: hdfsOpenFile
> Looking into it in near future sounds like a good plan and it probably shou
Changed to use the HdfsFileReader for reading. It can reuse the file handle. Thanks for the suggestion.
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@412
PS6, Line 412: int
> should be int64_t?
uses "int" because it should be enough for the number of the blocks per file (num_read_buffer_blocks_per_file_), which is also defined as "int".
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@426
PS6, Line 426: changed
> nit: reduced
Done
http://gerrit.cloudera.org:8080/#/c/17979/7/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/7/be/src/runtime/tmp-file-mgr.cc@1014
PS7, Line 1014: return read_buffer_file;
: }
> nit. Please add comment here this block of code which does several things.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Dec 2021 05:24:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9703/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Nov 2021 01:03:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@326
PS5, Line 326: int read_buffer_idx = offset / read_buffer_size();
> Can you pls double check?
I saw the IF check below which should take care of index being 12. Since the assigned idx is always the last entry, we will have a skew. A better approach is to use % num_of_read_buffers() to distribute it evenly.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Nov 2021 15:50:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
memory.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
12 files changed, 1,227 insertions(+), 150 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/7
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9769/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 20:57:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 7:
(4 comments)
Thanks a lot for the reply!
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: read_buffer_bloc, MemBlockStatus::DISABLED, dstl, &read_buffer_lock)) {
> I think there are two types of memory may be involved.
Agree that the memory for MemorBlock structures is not the concern.
So here, we check the status of a mem block and found it is DISABLED. We return right away. I guess my original point is still valid: why not reuse the block here?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@430
PS5, Line 430: RN_IF_ERROR(
> It is a good point, could be a potential performance improvement.
Thanks a lot for trying it out!
In this case, I wonder if it is sufficient to keep limit number of file handles open only for read.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@445
PS5, Line 445: queue->read_size()->Update(local_file_size);
: disk_file_dst_->SetReadBufferBlockStatus(
> It depends on how the caller deal with the error.
Done
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
PS6, Line 411: tmp_dirs_remote_ctrl_.tmp_file_pool_->dequeue_timer_metric_ =
: metrics->RegisterMetric(new HistogramMetric(
: MetricDefs::Get(TMP_FILE_BUFF_POOL_DEQUEUE_DURATI
> It doesn't need the memory for the entire file.
Can you explain what happens when the system limit is reached and we can't allocate the memory for the block?
My concern is about this use case.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 22:19:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 14:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc@191
PS13, Line 191: protected:
> Naming this "ValidateMemBlockStatus" seems more consistent with naming test
Done. Moved to DiskFileTest.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc@2494
PS13, Line 2494:
> ObjectPool is useful when you need to generate a bunch or variable number o
Done. Moved to DiskFileTest.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc@2525
PS13, Line 2525: ASSERT_EQ(0, status.code());
> Does disk-io-mgr-test contain a bunch of unit tests for DiskFile too? This
Added some negative tests. Moved to DiskFileTest.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@264
PS13, Line 264: disk_file_->UpdateReadBufferMetaDataIfNeeded(written_bytes - len_);
> The condition check is unnecessary because UpdateReadBufferMetaData returns
Done
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@353
PS13, Line 353: if (!status.ok()) goto end;
> nit: line length limit is 90, this doesn't need to wrap.
Done
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@407
PS13, Line 407: // Get the shared lock to prevent the physical files from deletion during the fetching.
> Storing the result of `c_str()` is usually a bad idea, since it has no guar
Done
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@411
PS13, Line 411: shared_lock<shared_mutex> srcl(disk_file_src_->physical_file_lock_);
> Initializing this up-front is a very old-school C idiom. Compilers can ofte
Done
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@447
PS13, Line 447: read_buffer_bloc, MemBlockStatus::WRITTEN, dstl, &read_buffer_lock);
> If we wanted to get fancy we could use a lambda to return the read result.
Done
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr-test.cc@2101
PS13, Line 2101: TEST_F(TmpFileMgrTest, TestBatchReadingFromRemote) {
> Can we add a test case where the remote file is deleted while DoUpload is i
Added a testcase DiskIoMgrTest::WriteToRemoteFileDeleted, but it is hard to delete the remote physical file in the middle of the uploading because the detection using hdfsExists seems only after the file is closed.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1019
PS13, Line 1019: // waiting for the fetch, so that it won't block the current page reading.
> So if MemBlock is reserved/allocated, that means another process has alread
Added comments.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1022
PS13, Line 1022: if (read_buffer_file->IsReadBufferBlockStatus(read_buffer_block,
> Ok, took me a sec to see why these checks make sense even though the only c
This part is changed a lot in part 2. Maybe we can optimize the structure in part 2.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 14
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Jul 2022 15:53:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 14: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 14
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 19 Jul 2022 16:56:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 15:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11077/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 15
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 02 Aug 2022 23:27:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 7: Code-Review+1
(6 comments)
Looks good!
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: read_buffer_bloc, MemBlockStatus::DISABLED, dstl, &read_buffer_lock)) {
> I think the mechanism is like this, any read(pin) who wants the block memor
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@430
PS5, Line 430: RN_IF_ERROR(
> Tried a way like closing the file handle if two blocks have used. The perfo
Looking into it in near future sounds like a good plan and it probably should be given a high priority.
In realty, say with the HJ builder use case, the overflow is very much instance specific and so is the tmp file itself. Therefore, only the reader(s) of that tmp file contend for the file handle.
Per this doc, hdfs lib API is thread safe: https://hadoop.apache.org/docs/r1.2.1/libhdfs.html.
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@412
PS6, Line 412: ist
should be int64_t?
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
PS6, Line 411: tmp_dirs_remote_ctrl_.tmp_file_pool_->dequeue_timer_metric_ =
: metrics->RegisterMetric(new HistogramMetric(
: MetricDefs::Get(TMP_FILE_BUFF_POOL_DEQUEUE_DURATI
> We will go "read by page" instead in this case. Because the "read by page"
Okay I see. Done.
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@426
PS6, Line 426: nique_i
nit: reduced
http://gerrit.cloudera.org:8080/#/c/17979/7/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/7/be/src/runtime/tmp-file-mgr.cc@1014
PS7, Line 1014: if (read_buffer_file->IsReadBufferBlockStatus(read_buffer_block,
: io::MemBlockStatus::UNINIT, read_file_lock, &mem_bloc_lock)) {
nit. Please add comment here this block of code which does several things.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Sat, 13 Nov 2021 00:31:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 3:
(16 comments)
Looks great and thanks a lot for the careful rework.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@55
PS1, Line 55:
> I am a little worried about the memory leak.
Got it.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@70
PS1, Line 70: DCHECK_EQ(static_cast<int>(status_), static_cast<int>(MemBlockStatus::RESERVED));
: // Use malloc, could be better to alloc from a buffer pool.
: data_ = static_cast<uint8_t*>(malloc(size));
: DCHECK(data_ != nullptr);
: SetAllocatedLocked(lock);
: }
:
: uint8_t* data() { return data_; }
:
> I would like to keep the caller away from passing the enum, therefore desig
That probably is fine to squeeze last ounce of performance. In impala server, a state enum is directly passed: https://github.com/apache/impala/blob/master/be/src/service/client-request-state.cc#L302
The code looks clean IMHO overall.
In addition, IsDisabledLocked() can be renamed to IsDisabled(). Since a lock argument is present, there is no ambiguity.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@193
PS1, Line 193: bloc
I see. It makes sense. As long as the disk file lock is a shared lock, this design will work.
Should we use std::shared_lock in some of the instance below?
[10:28:52 qchen@qifan-10229: src] grepit disk_file_lock_
runtime/io/disk-file.h: std::lock_guard<SpinLock> l(disk_file_lock_);
runtime/io/disk-file.h: std::lock_guard<SpinLock> l(disk_file_lock_);
runtime/io/disk-file.h: std::lock_guard<SpinLock> lock(disk_file_lock_);
runtime/io/disk-file.h: std::lock_guard<SpinLock> lock(disk_file_lock_);
runtime/io/disk-file.h: std::lock_guard<SpinLock> lock(disk_file_lock_);
runtime/io/disk-file.h: std::lock_guard<SpinLock> lock(disk_file_lock_);
runtime/io/disk-file.h: SpinLock disk_file_lock_;
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@195
PS1, Line 195: /// Members used when
Can we use MemBlockState state_ here?
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@292
PS1, Line 292:
: /// Returns the disk type o
> Changed the actual_file_size_ to ordinary int64_t. Just think maybe it is b
Okay. If the value is read only in nature, we only need to lock it before writing to it.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@393
PS1, Line 393: }
> It should be okay without the lock, because the check is to use the index t
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.cc@123
PS1, Line 123: *allocated = true;
> Added some comments for MemBlock in disk-file.h.
Okay. I see.
Maybe clarify as "The MemBlock must be in MemBlockStatus::DISABLED before calling ~MemBlock()."
May also add comment here to indicate fall through with each case is intentional.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-context.cc
File be/src/runtime/io/request-context.cc:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-context.cc@201
PS3, Line 201: unstarted_remote_file_oper_ranges_
Maybe named to unstarted_remote_file_op_ranges_?
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h
File be/src/runtime/io/request-ranges.h:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h@118
PS3, Line 118: FILE_FETCH
May need to explain what it is.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h@702
PS3, Line 702: do the file upload operation
nit. upload the file to a remote location.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/request-ranges.h@708
PS3, Line 708: the fetch file
nit. the fetch file operation from a remote site.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/scan-range.cc@171
PS3, Line 171: should be
: // guaranteed the range can be read in one round in this case.
Suggest to remove as there is no guarantee.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/io/scan-range.cc@178
PS3, Line 178: read_status
need to check the status.
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/3/be/src/runtime/tmp-file-mgr-test.cc@2101
PS3, Line 2101: TestBatchingReadFromRemote
Do we need a test on remote upload?
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@257
PS2, Line 257: Status setup_read_buffer_status = SetUpReadBufferParams();
: if (!setup_read_buffer_status.ok()) {
> Yes. That is the case in the new testcase, TmpFileMgrTest::TestBatchingRead
If handling the rare case is simple task, I feel we should do so.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@1039
PS2, Line 1039: new RemoteOperRange(disk_file_.get(), read_buffe
> Nope, in this version, we disable the buffer block if fails to reserve the
Will this impact the usefulness of this patch when there is a sudden memory pressure and later released?
For example, out of n buffer blocks, only 1/10 are able to get the memory before the arrival of the pressure. Assume the pressure is over quickly and a good amount of system memory is available.
Can the 9/l10 buffer blocks be useful from this point of time on?
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Oct 2021 15:38:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9693/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Oct 2021 00:28:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 15:
(9 comments)
Looks good to me. Some initial comments.
http://gerrit.cloudera.org:8080/#/c/17979/12//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17979/12//COMMIT_MSG@10
PS12, Line 10: file in order to improve the reading performance for the spilled
nit: reading -> read
http://gerrit.cloudera.org:8080/#/c/17979/12//COMMIT_MSG@13
PS12, Line 13: Originally, the design is to use the local disk file as the buffer
nit: is -> was
http://gerrit.cloudera.org:8080/#/c/17979/14/be/src/runtime/io/disk-file-test.cc
File be/src/runtime/io/disk-file-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/14/be/src/runtime/io/disk-file-test.cc@30
PS14, Line 30: MemBlockStatus new_status, bool expect_suc);
'expect_success' is probably better for reading.
http://gerrit.cloudera.org:8080/#/c/17979/14/be/src/runtime/io/disk-file-test.cc@33
PS14, Line 33: // last_status is MemBlock's last status is going to reach other than
missing "it" in the description.
last_status is MemBlock's last status 'it' is going to reach other than
http://gerrit.cloudera.org:8080/#/c/17979/14/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/14/be/src/runtime/io/disk-file.h@45
PS14, Line 45: /// UNINIT is the default status, indicates the block is not uninitialized.
typo?
not uninitialized. => not initialized.
http://gerrit.cloudera.org:8080/#/c/17979/14/be/src/runtime/io/disk-file.h@72
PS14, Line 72: // Allcate the memory for the MemBlock.
typo "Allcate"
http://gerrit.cloudera.org:8080/#/c/17979/15/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/15/be/src/runtime/tmp-file-mgr.cc@604
PS15, Line 604: admit_mem_limit
I believe this is the memory limit for query processing, which is what I would expect we use and not use the 20% emergency memory. But, then this contradicts the commit message.
http://gerrit.cloudera.org:8080/#/c/17979/15/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/17979/15/common/thrift/metrics.json@2424
PS15, Line 2424: The used read memory buffer of all scratch directories
Maybe re-word this as follows to reflect this is current usage?
"The current total read memory buffer bytes for all scratch directories"
http://gerrit.cloudera.org:8080/#/c/17979/15/common/thrift/metrics.json@2434
PS15, Line 2434: The high water mark for read memory buffer of all scratch directories.
Better wording to include the unit/bytes:
The high water mark for read memory buffer bytes of all scratch directories.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 15
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 23:25:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 18:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17979/15/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/15/be/src/runtime/tmp-file-mgr.cc@604
PS15, Line 604:
> I believe this is the memory limit for query processing, which is what I wo
Thanks for pointing this out. Made a change on the max allowed bytes for read buffer. Now is 10% of the process bytes limit. Also modified the commit message.
http://gerrit.cloudera.org:8080/#/c/17979/15/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/17979/15/common/thrift/metrics.json@2424
PS15, Line 2424: The current total read memory buffer bytes for all scr
> Maybe re-word this as follows to reflect this is current usage?
Done
http://gerrit.cloudera.org:8080/#/c/17979/15/common/thrift/metrics.json@2434
PS15, Line 2434: The high water mark for read memory buffer bytes of all scratch direct
> Better wording to include the unit/bytes:
Done
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 18
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Aug 2022 23:19:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 21:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11347/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 17:55:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 20:
Rebased and refined the limits for the read buffer based on Abhishek's comments.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 20
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 12 Sep 2022 17:17:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
system memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
14 files changed, 1,279 insertions(+), 151 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/12
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 12
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 10:
The patch optimizes the mechanism of fetching the blocks, now there will be no waits for the block fetching. This change shortens the query time of tpcds-q78, which was found around 10% regression compared to the "no batch" in patch 7.
Below is the new performance results on tpcds-q78, both run for three rounds and the fastest one would be the result.
Batch No Batch
9m48s 10m5s
More optimizations on batch reading will be in IMPALA-11064.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 10
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Feb 2022 05:21:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 6:
(6 comments)
We have at most 10% of the process memory for the remote batching reading, theoretically this memory consumption should be okay, but maybe later we can test more fierce cases
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: read_buffer_bloc, MemBlockStatus::DISABLED, dstl, &read_buffer_lock)) {
> Well, if we are here, it means we want to a memory block to hold some data,
I think there are two types of memory may be involved.
For the memory of the block content, we don't need to allocate all of them of a file during construction.
Each block will reserve or allocate its own memory separately when a page in the block is going to be read(pinned).
Right now the memory reuse process is, we will free the memory of a DISABLED block and return the reservation to the global, then other block can get the reservation and allocate itself. Maybe later we can have some memory pool, but compared to the rate of network transmission, it should be not that bad to have one system free and malloc during reuse.
For the memory of block structures(MemBlock), we will have all of them since construction of the TmpFile till destruction. For example, if the file is 256M, block is 16M, we will have 16 block structures created since construction of the TmpFile. But consider that, the size of each MemBlock structure is 128 bytes, if we are going to spill 1TB data, the total memory needs for the block structures is 1T/16M*128B=8M, the number is not that bad.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@430
PS5, Line 430: RN_IF_ERROR(
> This could be a performance hit to open/close a file to read one data block
It is a good point, could be a potential performance improvement.
I modified the code and did some tests, looks like the reuse of the hdfs file handle doesn't work that good. Firstly, the hdfsFile seems not allow multithreading use, therefore, there needs to be an exclusive lock among multiple blocks in the same file, the performance is affected. Secondly, it could require more memory because probably we need to keep all the file handle till query ends, during the tests, I have met hanging on the the calls to hdfs interface after lots of file handle have been created, I assume it could be related.
Therefore, to keep the file open, may need more efforts, could be good to put it in the next round of improvement.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@445
PS5, Line 445: queue->read_size()->Update(local_file_size);
: disk_file_dst_->SetReadBufferBlockStatus(
> See my comment above. Basically, I have a good mem block and there is a rea
It depends on how the caller deal with the error.
Right now, if fails to fetch the block, we simply return the block memory to global(the block structure is still there, but content memory is released), therefore some other block can use. The one who fails to fetch a block will change the policy to read by page for this page. If read by page fails again, the query will fail.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@452
PS5, Line 452:
> This code could mask out a read error set at line 445. Better to return th
Done
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@402
PS6, Line 402: TmpFileMgr::SetUpReadBufferParams
> Since the work in this method is done for tmp_dirs_remote_ctrl_, suggest to
Done
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
PS6, Line 411: tmp_dirs_remote_ctrl_.num_read_buffer_blocks_per_file_ =
: static_cast<int>(tmp_dirs_remote_ctrl_.remote_tmp_file_size_
: / tmp_dirs_remote_ctrl_.read_buffer_block_size_);
> This assumes that there is enough memory to hold the entire file. When it i
It doesn't need the memory for the entire file.
During the initialization, we only create the structure of the memory blocks, not reserve or allocate the memory for the block content.
Each block will reserve or allocate its own memory separately.
We have a global limitation on the sum of the size of all the blocks, which is the tmp_dirs_remote_ctrl_.max_read_buffer_size_. The global usage of the read buffer memory is tracked by current_read_memory_allocated_ in https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/tmp-file-mgr.h@400.
If the current global usage is below the limitation, the block will reserve the memory successfully. The memory reservation for a block is done in https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@1024, when someone wants to pin a page.
Then with the reservation, the block can allocate the memory in https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/io/disk-io-mgr.cc@430. The data will be read from the remote file to the block memory which was just allocated.
If error happens or query ends, the memory of the block will be released according to the status of the block in https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@1107. By decreasing the reservation tracker current_read_memory_allocated_ by one block size, other blocks could be able to reserve the memory then allocate.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Nov 2021 21:14:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
system memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
14 files changed, 1,278 insertions(+), 151 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/10
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 10
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 6:
(10 comments)
Looks very good!
Another thing we may need to worry about is the enhancement to FE to include the extra system memory blocks proposed to use in this patch for spill-able nodes, such as HJB, SORT or ANALYTICEVAL nodes.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@206
PS5, Line 206:
: virtual ~DiskFile() {}
> Previously, a read buffer and a memory block are the same thing in our patc
Okay. That is a good reason.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@326
PS5, Line 326: id UpdateReadBufferMetaData(int64_t offset) {
> It depends on the file size.
Please see my comments at https://gerrit.cloudera.org/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.cc@116
PS5, Line 116:
> Think for a while, maybe DISABLED is better, because it means the block doe
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@424
PS5, Line 424: read_buffer_bloc, MemBlockStatus::DISABLED, dstl, &read_buffer_lock)) {
> Because the memory of the block is released, if this case happens, we can't
Well, if we are here, it means we want to a memory block to hold some data, and query is not cancelled or all pages of the block are read, right?
The transition here is actually from DISABLED back to ALLOC to facilitate a reuse.
I am thinking that there are limited number of mem blocks to use (say 10) that are not enough to cover the entire file. See my other comment on when the available memory from system is not enough for the entire file.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@430
PS5, Line 430: RN_IF_ERROR(
This could be a performance hit to open/close a file to read one data block. Is it possible to keep the file open?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@436
PS5, Line 436: atus = Status(TErrorCode::DISK_IO_ERROR, GetBackendStri
> The read timer works when it is destructed. It counts the time interval dur
Done
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@445
PS5, Line 445: queue->read_size()->Update(local_file_size);
: disk_file_dst_->SetReadBufferBlockStatus(
> Do you mean delete the memory block if fails? Right now it is handled by th
See my comment above. Basically, I have a good mem block and there is a read error for a particular offset. The question is whether we can use the block on other offsets.
But if the reading error here is fatal (say for HJ builder, we can't read some block of data), then we should terminate the execution for the query.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-io-mgr.cc@452
PS5, Line 452:
This code could mask out a read error set at line 445. Better to return the read error.
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@402
PS6, Line 402: TmpFileMgr::SetUpReadBufferParams
Since the work in this method is done for tmp_dirs_remote_ctrl_, suggest to move it to class TmpDirRemoteCtrl.
http://gerrit.cloudera.org:8080/#/c/17979/6/be/src/runtime/tmp-file-mgr.cc@411
PS6, Line 411: tmp_dirs_remote_ctrl_.num_read_buffer_blocks_per_file_ =
: static_cast<int>(tmp_dirs_remote_ctrl_.remote_tmp_file_size_
: / tmp_dirs_remote_ctrl_.read_buffer_block_size_);
This assumes that there is enough memory to hold the entire file. When it is not the case as handled at line 424 below, I wonder if we need to adjust num_read_buffer_blocks_per_file_ accordingly. A different strategy is to take the max allowed memory 'max_allow_bytes' and use it to figure out num_read_buffer_blocks_per_file_.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 10 Nov 2021 14:56:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 1:
(37 comments)
http://gerrit.cloudera.org:8080/#/c/17979/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17979/1//COMMIT_MSG@38
PS1, Line 38: now the system
> You mean Impala or OS?. Is it a guarantee?
changed, should be the impala process memory. The mem_limit should be 80% by default.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@47
PS1, Line 47: DONE
> nit. WRITTEN probably is better.
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@55
PS1, Line 55: DISABLED
> nit. Could be UNINIT too?
I am a little worried about the memory leak.
So the final state of the memory block needs to be MemBlockStatus::DISABLED, and every block needs to be called Delete() before destroyed to prevent any memory not released.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@62
PS1, Line 62: poool
> nit.
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@70
PS1, Line 70: bool IsDisabled() {
: std::unique_lock<SpinLock> l(lock_);
: return IsDisabledLocked(l);
: }
:
: bool IsDisabledLocked(const std::unique_lock<SpinLock>& lock) {
: DCHECK(lock.mutex() == &lock_ && lock.owns_lock());
: return status_ == MemBlockStatus::DISABLED;
: }
> Can this pair of methods to be simplified as follows?
I would like to keep the caller away from passing the enum, therefore design it this way, the benefit is that the caller doesn't need to pass the enum.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@193
PS1, Line 193: lock_
> A little bit confusing when read code which applies lock_. since there are
Changed the name to memory_block_lock_.
I think the benefit of using the memory block is that it may reduce potential contention. For example, if using disk file lock, which is a spinlock, only one block out of four (or more) can be written at a time, and also the status changing.
The rule now is the disk lock is to protect the members actual_file_size_ and read_buffer_ctrl_ (exclude the memory blocks). The members of memory blocks are protected by their own locks.
If the caller worries the memory block to be destructed while using, the caller can first hold the physical_file_lock_, which is a shared lock, to protect the while file from destruction.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@292
PS1, Line 292: std::lock_guard<SpinLock> l(lock_);
: return actual_file_size_;
> actual_file_size_ is AtomicInt64. Do we need to lock with lock_ here first?
Changed the actual_file_size_ to ordinary int64_t. Just think maybe it is better to be managed by the lock_ like others.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@379
PS1, Line 379: num_of_read_buffer
> nit. num_of_read_buffers.
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@381
PS1, Line 381: equals
> nit equal
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@391
PS1, Line 391: DCHECK_LT(buffer_idx, num_of_read_buffer());
: DCHECK_GE(buffer_idx, 0);
> Should call DCheckReadBufferIdx().
Thanks, done.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@393
PS1, Line 393: std::lock_guard<SpinLock> lock(lock_);
> should we lock it first before check?
It should be okay without the lock, because the check is to use the index to compare with constant variables.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@415
PS1, Line 415: DCheckReadBufferIdx(buffer_idx);
: std::lock_guard<SpinLock> lock(lock_);
> Lock before check?
same as above.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@509
PS1, Line 509: lock_
> Suggest to rename as disk_file_lock_.
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.cc@123
PS1, Line 123: *allocated = true;
> If the memory is deleted, the state in its control block should become rese
Added some comments for MemBlock in disk-file.h.
The MemBlock must be in MemBlockStatus::DISABLED before destruction, and the Delete() would be needed to call before destruction and it will set the state to MemBlockStatus::DISABLED for all cases.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr-internal.h
File be/src/runtime/tmp-file-mgr-internal.h:
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr-internal.h@234
PS2, Line 234: is
> nit if
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr-internal.h@339
PS2, Line 339: // Return the
> nit. incomplete.
Thanks. Forgot this one. Done.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h
File be/src/runtime/tmp-file-mgr-internal.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h@233
PS1, Line 233: of
> nit off.
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h@234
PS1, Line 234: is
> nit if.
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h@283
PS1, Line 283: SetToDelete
> nit. May name as SetToDeleteFlag() since the argument can be false which in
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@146
PS2, Line 146: block
> nit memory block?
changed to use "read buffer (memory block)" to describe the block.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@149
PS2, Line 149: The number of read buffer files
> nit. Should it be number of buffer blocks, since we are dividing file size
change to use "read buffer (memory block)" to describe the block.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@151
PS2, Line 151: remote_tmp_file_read_buffer_num_
> nit. may be num_remote_read_buffers_?
changed to remote_num_read_buffers_per_file_.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@158
PS2, Line 158: remote_read_memory_buffer_size_
> nit maybe max_remote_read_buffer_size_?
Changed to remote_max_total_read_buffer_size_. Keep the remote as prefix.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@166
PS2, Line 166: remote_batching_read_
> nit. remote_batching_read_enabled_
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@220
PS2, Line 220: GetRemoteTmpFileReadBuffNum
> GetNumRemoteReadBuffers()
Changed to GetRemoteNumReadBuffersPerFile(), want to keep the Remote as a prefix, so that we can easily to tell the function is working for the remote fs.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@225
PS2, Line 225: GetRemoteReadMemoryBuffSize
> Buffer instead Buff?
changed to GetRemoteMaxTotalReadBufferSize.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.h@292
PS2, Line 292: helps to release
> releases
Done
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr.h@245
PS1, Line 245: Return if batching read for remote temporary files is enabled.
> nit. Indicate ...
Changed to IsRemoteBatchingReadEnabled().
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@107
PS2, Line 107: read memory buffer
> read buffers
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@144
PS2, Line 144:
> the maximal percentage of the system memory that can be utilized
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@257
PS2, Line 257:
: tmp_dirs_remote_ctrl_.remote_batching_read_ = FLAGS_re
> Should we consider a special case here when the remote temp file size is sm
Yes. That is the case in the new testcase, TmpFileMgrTest::TestBatchingReadFromRemote.
The testcase is using 1KB each file size, and therefore each block is 256B. But the page is 512B. Therefore, there are only two blocks with holes for each file like below:
blocks [512B] [] [512B] []
start offset 0 256 512 768
It should be working fine.
But since it is a very rare case, the max page size of Impala is 2MB, unless we set the file size lower than 8MB, we should not have this case.
In current production system, we are using 256MB for each file.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@260
PS2, Line 260: rs_remote_ctrl_.remote_read_buffer_block_size_ =
> Should create a constant for it as the number in bytes is computed twice.
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@257
PS2, Line 257:
: tmp_dirs_remote_ctrl_.remote_batching_read_ = FLAGS_remote_batching_read;
: if (tmp_dirs_remote_ctrl_.remote_batching_read_) {
: tmp_dirs_remote_ctrl_.remote_read_buffer_block_size_ =
: tmp_dirs_remote_ctrl_.remote_tmp_file_size_ / 4;
: if (tmp_dirs_remote_ctrl_.remote_read_buffer_block_size_
: > MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB * 1024 * 1024) {
: tmp_dirs_remote_ctrl_.remote_read_buffer_block_size_ =
: MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB * 1024 * 1024;
: }
: tmp_dirs_remote_ctrl_.remote_tmp_file_read_buffer_num_ =
: static_cast<int>(tmp_dirs_remote_ctrl_.remote_tmp_file_size_
: / tmp_dirs_remote_ctrl_.remote_read_buffer_block_size_);
: tmp_dirs_remote_ctrl_.remote_read_memory_buffer_size_ =
: ParseUtil::ParseMemSpec(FLAGS_remote_read_memory_buffer_size, &is_percent, 0);
: i
> This entire block of code is a good candidate for a member function to popu
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@278
PS2, Line 278: 4_t sys_bytes_limit
> Should check max_allow_bytes > 0
Done
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@954
PS2, Line 954: _, file_group_->tmp_file_mgr_->GetRemoteTmpFileSi
> We should check disk_read_page_cnts_.get() is not null.
Added a DCHECK.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@1039
PS2, Line 1039: }
> Do we revive this buffer block when the memory pressure is back to normal?
Nope, in this version, we disable the buffer block if fails to reserve the space, so the pages belongs to the block will be read by page. But because there are other blocks, they can still use the buffer block when the memory pressure is back to normal.
http://gerrit.cloudera.org:8080/#/c/17979/2/be/src/runtime/tmp-file-mgr.cc@1074
PS2, Line 1074:
> IncrementReadPageCount() since we increase the count by 1.
Done
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 29 Oct 2021 02:34:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 22:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8599/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 22
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 22 Sep 2022 16:10:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 23:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8629/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 23
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 28 Sep 2022 03:50:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
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/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch read from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page. If yes, the block will
be stored in the memory until all of the pages in the block are
read or the query ends. If not, we will go reading the page
directly and disable this block, because it may be good to avoid
duplicated reads from the remote fs for the same content.
One challenge of the read buffer is where to get the extra memory
for it, because when impala starts to spill data, it means the
process lacks of memory to use. By default, impala process will
reserve 20% of the total system memory as unused memory, and here
we will use this unused memory for the read buffer because it is
reasonable to use it for the emergency case like spilling and
the memory of the read buffer will be returned immediately after
the use.
For system reliability consideration, we set a restriction that,
the maximum bytes of the read buffer memory are no more than 10%
of the total system memory and 50% of the unused memory. Also,
if the unused memory is less than 5% of the total system memory,
the read buffer will be disabled.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number is also restricted
by the process memory limit, which can not exceed 10% of the process
memory limit.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Reviewed-on: http://gerrit.cloudera.org:8080/17979
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/mem-info.cc
M be/src/util/mem-info.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
19 files changed, 1,666 insertions(+), 156 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 24
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10945/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 14
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 11 Jul 2022 15:44:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 21: Code-Review+1
(1 comment)
Looks great! Thanks Yida for the careful rework.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1078
PS13, Line 1078: [read_buffer_block, tmp_file = this](const Status& fetch_status) {
> The function firstly check whether the local buffer exists (even batch read
Done
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 14 Sep 2022 13:13:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#21). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch read from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page. If yes, the block will
be stored in the memory until all of the pages in the block are
read or the query ends. If not, we will go reading the page
directly and disable this block, because it may be good to avoid
duplicated reads from the remote fs for the same content.
One challenge of the read buffer is where to get the extra memory
for it, because when impala starts to spill data, it means the
process lacks of memory to use. By default, impala process will
reserve 20% of the total system memory as unused memory, and here
we will use this unused memory for the read buffer because it is
reasonable to use it for the emergency case like spilling and
the memory of the read buffer will be returned immediately after
the use.
For system reliability consideration, we set a restriction that,
the maximum bytes of the read buffer memory are no more than 10%
of the total system memory and 50% of the unused memory. Also,
if the unused memory is less than 5% of the total system memory,
the read buffer will be disabled.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number is also restricted
by the process memory limit, which can not exceed 10% of the process
memory limit.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/mem-info.cc
M be/src/util/mem-info.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
19 files changed, 1,666 insertions(+), 156 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/21
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9997/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 9
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 10 Jan 2022 23:42:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10112/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 10
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Feb 2022 00:51:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9887/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 07 Dec 2021 05:04:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 8:
Great. I think it will be helpful to study how an operator spills and reads back the spilled data and then tailor the temporary files accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 08 Dec 2021 14:44:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10293/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 12
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 15 Mar 2022 21:49:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#18). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch read from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of the read buffer is where to get the extra memory
for it, because when impala starts to spill data, it means the
process lacks of memory to use. By default, impala process will
reserve 20% of the total system memory as unused memory, and here
we decide to use this unused memory for the read buffer because
it is reasonable to use it for the emergency case like spilling.
The maximum bytes of the read buffer memory are 10% of the process
memory limit, which is a conservative number compared to 20% of
the total system memory. The read buffer size plus process memory
limit should not exceed the total system memory.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number is also restricted
by the process memory limit, which can not exceed 10% of the process
memory limit.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/mem-info.cc
M be/src/util/mem-info.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
19 files changed, 1,587 insertions(+), 161 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/18
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 18
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 18:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/11214/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 18
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 23 Aug 2022 23:15:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 18: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 18
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 29 Aug 2022 17:16:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 21: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Sep 2022 22:09:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 22:
The dry run should hit "IMPALA-11508 Iceberg test test_expire_snapshots is flaky"
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 22
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 28 Sep 2022 03:14:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 21: Code-Review+2
+2 to carry Michael and Abhishek's vote.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 21 Sep 2022 14:10:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
system memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
15 files changed, 1,340 insertions(+), 151 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/13
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 13
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 1:
(18 comments)
Just try to understand this piece of work. Sounds promising.
http://gerrit.cloudera.org:8080/#/c/17979/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17979/1//COMMIT_MSG@38
PS1, Line 38: now the system
You mean Impala or OS?. Is it a guarantee?
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@47
PS1, Line 47: DONE
nit. WRITTEN probably is better.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@55
PS1, Line 55: DISABLED
nit. Could be UNINIT too?
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@62
PS1, Line 62: poool
nit.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@70
PS1, Line 70: bool IsDisabled() {
: std::unique_lock<SpinLock> l(lock_);
: return IsDisabledLocked(l);
: }
:
: bool IsDisabledLocked(const std::unique_lock<SpinLock>& lock) {
: DCHECK(lock.mutex() == &lock_ && lock.owns_lock());
: return status_ == MemBlockStatus::DISABLED;
: }
Can this pair of methods to be simplified as follows?
bool IsDisabled() {
std::unique_lock<SpinLock> l(lock_);
return IsInStateLocked(l, MemBlockStatus::DISABLED);
}
bool IsInStateLocked(const std::unique_lock<SpinLock>& lock, enum x) {
DCHECK(lock.mutex() == &lock_ && lock.owns_lock());
return status_ == x;
}
In this way, other similar methods can call IsInStateLocked() directly.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@193
PS1, Line 193: lock_
A little bit confusing when read code which applies lock_. since there are two lock_ in two classes.
Suggest to name the lock_ here to memory_block_lock_.
I also wonder if we can just use the lock in disk file instead.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@292
PS1, Line 292: std::lock_guard<SpinLock> l(lock_);
: return actual_file_size_;
actual_file_size_ is AtomicInt64. Do we need to lock with lock_ here first?
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@379
PS1, Line 379: num_of_read_buffer
nit. num_of_read_buffers.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@381
PS1, Line 381: equals
nit equal
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@391
PS1, Line 391: DCHECK_LT(buffer_idx, num_of_read_buffer());
: DCHECK_GE(buffer_idx, 0);
Should call DCheckReadBufferIdx().
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@393
PS1, Line 393: std::lock_guard<SpinLock> lock(lock_);
should we lock it first before check?
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@415
PS1, Line 415: DCheckReadBufferIdx(buffer_idx);
: std::lock_guard<SpinLock> lock(lock_);
Lock before check?
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.h@509
PS1, Line 509: lock_
Suggest to rename as disk_file_lock_.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/io/disk-file.cc@123
PS1, Line 123: *allocated = true;
If the memory is deleted, the state in its control block should become reserved at most.
Suggest to also update its control block in this method.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h
File be/src/runtime/tmp-file-mgr-internal.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h@233
PS1, Line 233: of
nit off.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h@234
PS1, Line 234: is
nit if.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr-internal.h@283
PS1, Line 283: SetToDelete
nit. May name as SetToDeleteFlag() since the argument can be false which indicates the files are not deleted.
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:
http://gerrit.cloudera.org:8080/#/c/17979/1/be/src/runtime/tmp-file-mgr.h@245
PS1, Line 245: Return if batching read for remote temporary files is enabled.
nit. Indicate ...
May rename as CanRemoteBatchingRead()
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Oct 2021 18:34:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly, but in order to guarantee a page not
being cut into two parts in different blocks, the block size
could be a little different to each other in practice. The default
block size is the minimum value between 1/4 default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
5% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 5% of the total memory.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
12 files changed, 1,182 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/5
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Qifan Chen (Code Review)" <ge...@cloudera.org>.
Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
Patch Set 5:
(14 comments)
Looks pretty good.
I think my two concerns are resolved after this morning's discussion.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@52
PS5, Line 52: class MemBlock
Need a description for MemBlock class.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@62
PS5, Line 62: if the memory is reserved or allocated before deletion to the caller
nit. Return whether the memory is reserved or allocated before deletion to the caller
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@68
PS5, Line 68: int64_t size, const std::unique_lock<SpinLock>& lock)
nit. may switch the order of the two arguments as the lock appears first in other similar calls.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@168
PS5, Line 168: /// The data of the memory block.
nit. May describe the content of a memory block a little bit. For example, a memory block contains multiple pages.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@200
PS5, Line 200: DiskFile(const std::string& path, DiskIoMgr* io_mgr, int64_t file_size,
nit missing the comment.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@206
PS5, Line 206: DiskFileReadBuffCtrl
Since this class is nested within DiskFile class, suggest to call it ReadBuffer which owns a number of memory blocks.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@206
PS5, Line 206:
: DiskFileReadBuffCtrl(int64_t
A little bit confusing on how these two names relate to mem blocks. See my two comments below.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@210
PS5, Line 210: read_buffer_size_
Suggest to use a name relate to memory block, say mem_block_size_?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@213
PS5, Line 213: num_of_read_buffers_
suggest to rename to num_mem_blocks_.
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.h@326
PS5, Line 326: int read_buffer_idx = offset / read_buffer_size();
Can you pls double check?
Assume the memory block size is 8MB, and the offset is 99MB. The idx = 99/8 = 12. If there are only 4 mem blocks, then this idx of 12 is invalid.
Am I missing something?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/io/disk-file.cc@116
PS5, Line 116: DISABLED
Since the memory is actually deleted in this method, maybe we should call the final state DELETED?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h@147
PS5, Line 147: remote_read_buffer_size_
nit. This name sounds like the buffer is remote. Maybe renamed as mem_block_size_?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h@151
PS5, Line 151: remote_num_read_buffers_per_file_
similar comment as above. num_mem_blocks_per_file instead?
http://gerrit.cloudera.org:8080/#/c/17979/5/be/src/runtime/tmp-file-mgr.h@158
PS5, Line 158: remote_max_total_read_buffer_size_
max_mem_block_size_?
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Nov 2021 20:09:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batching reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batching reading for remote temporary files
......................................................................
IMPALA-10791 Add batching reading for remote temporary files
The patch adds a feature to batching read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batching reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_MB, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batching_read. Default is false. If set true, the batching
read is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
system memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchingReadFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
13 files changed, 1,306 insertions(+), 149 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/9
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 9
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 12:
Patch12 changes the name of the option from remote_batching_read to remote_batch_read to shorten the name, and also related naming.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 12
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 15 Mar 2022 21:32:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#15). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch reading from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of using the memory for the buffer is that, when the
system is lacking of memory when it needs to spill the data. So we
make a restriction to limit the percentage of the memory for the
read buffer to 10% of the total, because right now the impala
process will reserve 20% memory as unused memory by default, using
10% for the emergency case like spilling is reasonable.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number also restricted by
the total system memory, which can not exceed 10% of the total
system memory.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
17 files changed, 1,572 insertions(+), 159 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/15
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 15
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#16). ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
IMPALA-10791 Add batch reading for remote temporary files
The patch adds a feature to batch read from a remote temporary
file in order to improve the reading performance for the spilled
remote data.
Originally, the design is to use the local disk file as the buffer
for batch read from the remote file. But in practice, it
doesn't help to improve the performance. Therefore, the design
is changed to use the memory as the read buffer.
Currently, each TmpFileRemote has two DiskFile, one is for the
remote, and one is for the local buffer. The patch adds MemBlocks
to the local buffer file. Each local buffer file is divided into
several MemBlocks evenly. Moreover, in order to guarantee a
single page not being cut into two parts in different blocks,
the block size could be a little different to each other in
practice. The default block size is the minimum value between
the default file size and
MAX_REMOTE_READ_MEM_BLOCK_THRESHOLD_BYTES, which is 16MB.
When pinning a page, the system will detect if there is enough
memory for the block that holds the page, if not, we will go
reading the page directly and disable this block, because it may
be good to avoid duplicated reads from the remote fs for the same
content. If the system decides to fetch a block, the block will be
stored in the memory until all of the pages in the block are read
or the query ends.
One challenge of the read buffer is where to get the extra memory
for it, because when impala starts to spill data, it means the
process lacks of memory to use. By default, impala process will
reserve 20% of the total system memory as unused memory, and here
we decide to use this unused memory for the read buffer because
it is reasonable to use it for the emergency case like spilling.
The maximum bytes of the read buffer memory are 10% of the process
memory limit, which is a conservative number compared to 20% of
the total system memory. The read buffer size plus process memory
limit should not exceed the total system memory.
Two start options have been added for the new feature.
1. remote_batch_read. Default is false. If set true, the batch read
is enabled.
2. remote_read_memory_buffer_size. Default is 1G. The maximum memory
that can be used by the read buffer. The number is also restricted
by the process memory limit, which can not exceed 10% of the process
memory limit.
Added metrics ScratchReadsUseMem/ScratchBytesReadUseMem/
ScratchBytesReadUseLocalDisk to the query profile.
The patch also increases the MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB
from 256 to 512.
Tests:
Ran core and exhaustive tests.
Added and ran TmpFileMgrTest::TestBatchReadingFromRemote.
Added e2e test test_scratch_dirs_batch_reading.
Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
---
M be/src/runtime/io/CMakeLists.txt
A be/src/runtime/io/disk-file-test.cc
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-file.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-context.cc
M be/src/runtime/io/request-context.h
M be/src/runtime/io/request-ranges.h
M be/src/runtime/io/scan-range.cc
M be/src/runtime/tmp-file-mgr-internal.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/mem-info.cc
M be/src/util/mem-info.h
M be/src/util/metrics.h
M common/thrift/metrics.json
M tests/custom_cluster/test_scratch_disk.py
19 files changed, 1,587 insertions(+), 161 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/17979/16
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 16
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 21:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17979/20//COMMIT_MSG@45
PS20, Line 45: the maximum bytes of the read buffer memory are no more than 10%
> nit. May mention that the memory borrowed from the reserved memory will be
Done
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 21
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 17:58:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 12:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@55
PS12, Line 55: class MemBlock {
> This seems like a fairly leaky abstraction. It mostly exists to handle the
It is quite a simple class working closely with the DiskFile and TmpFile stuff, may rely on the TmpFile stuff to reserve the memory (which relies on a global variable in TmpFileMgr) for it before having the right to allocate, and rely on the IO functions to write and read directly on the data_ under certain block status. Think for a while for a better abstraction, but dont have much idea for now while guarantee the efficiency and safety without too much complexity. I think one of the good thing here is the status transition is clear and one-way down, what the user needs to do, in most cases, is to lock on the block spinlock, do something, change the status if needed.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@220
PS12, Line 220: std::unique_ptr<int64_t[]> page_cnts_per_block_;
> This seems a little funny, but I think I see why it's necessary. A const ve
Maybe personal preference. Prefer a fixed-size array if the size is fixed and type is simple. Looks more clear.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@446
PS12, Line 446: // Helper function to check the status of a read buffe block.
> nit: buffe -> buffer
Done
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@458
PS12, Line 458: // Helper function to delete the read buffe block.
> nit: buffe -> buffer
Done
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@516
PS12, Line 516: /// The lock also protects the memory blocks from destruction, if the disk file has.
> "if the disk file has" seems like an incomplete sentence.
Done
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.cc@99
PS12, Line 99: void MemBlock::Delete(bool* reserved, bool* allocated) {
> Some unit tests around MemBlock transitions might be useful.
Added a unit testcase "MemBlockTest" should have covered the MemBlock status transitions.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/request-ranges.h
File be/src/runtime/io/request-ranges.h:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/request-ranges.h@152
PS12, Line 152: RequestRange(RequestType::type request_type, int disk_id = -1, int64_t offset = -1)
> How does an offset of -1 differ from 0?
I think the change is to allow assigning the offset in the constructor function. -1 is an invalid value for the offset, that means when we need to set the offset of the range before using the range, there are some assertion about it like "DCHECK_GE(offset, 0);" when using it.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc@156
PS12, Line 156: if (!use_mem_buffer) {
> Since we asserted above that use_local_buff implies !use_mem_buffer, this c
Added some comments. The logic here is to set the correct file reader if it involves reading from the file system (could be local or remote file). If it uses memory buffer, it doesn't need to set this. There is a case if use_mem_buffer and use_local_buff are all false, this case we will use the original file_reader_ to get the range.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc@291
PS12, Line 291: // 1. If it is the local buffer file is not deleted(evicted) yet.
> Change to "If the local buffer file ..."
Done
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr-test.cc@1835
PS12, Line 1835: int64_t file_size = 512 * 1024 * 1024;
> Why was this changed?
The maximum allowed file size for one remote file increases to 512MB in this patch defined in MAX_REMOTE_TMPFILE_SIZE_THRESHOLD_MB in tmp-file-mgr.cc. The reason for the increase is a bigger file size may have a better upload performance. By default we are still using 256MB, but may give users a chance if they would like to try a bigger size.
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/tmp-file-mgr.cc@117
PS12, Line 117: "Set if the system uses batch reading for the remote temporary files.");
> This description could be more descriptive. Maybe "Set to prefetch addition
Added some description. But the prefetch is only in part 2, will add the prefetch description in part 2.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 12
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 28 Jun 2022 00:46:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10791 Add batch reading for remote temporary files
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/17979 )
Change subject: IMPALA-10791 Add batch reading for remote temporary files
......................................................................
Patch Set 13:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h
File be/src/runtime/io/disk-file.h:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@55
PS12, Line 55: /// The caller may need to maintain the status of the MemBlock and make sure the block is
> It is quite a simple class working closely with the DiskFile and TmpFile st
Ack
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/disk-file.h@220
PS12, Line 220: SpinLock read_buffer_ctrl_lock_;
> Maybe personal preference. Prefer a fixed-size array if the size is fixed a
Ack
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc@191
PS13, Line 191: void TestMemBlockStatus(MemBlockStatus last_status);
Naming this "ValidateMemBlockStatus" seems more consistent with naming test helper functions.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc@2494
PS13, Line 2494: ObjectPool pool;
ObjectPool is useful when you need to generate a bunch or variable number of objects that need to persist passed the creation scope. That's not the case here, you could use a local variable, as in
MemBlock block(block_id);
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr-test.cc@2525
PS13, Line 2525: TEST_F(DiskIoMgrTest, MemBlockTest) {
Does disk-io-mgr-test contain a bunch of unit tests for DiskFile too? This would make more sense to me in a new file: disk-file-test.cc.
I'd also be interested in negative tests, but it looks like we only enforce order via DCHECK so the negative tests wouldn't really work.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@264
PS13, Line 264: if (disk_file_->IsBatchReadEnabled()) {
The condition check is unnecessary because UpdateReadBufferMetaData returns immediately when !IsBatchReadEnabled.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@353
PS13, Line 353: // is_to_delete flag.
nit: line length limit is 90, this doesn't need to wrap.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@407
PS13, Line 407: const char* remote_file_path = disk_file_src_->path().c_str();
Storing the result of `c_str()` is usually a bad idea, since it has no guaranteed lifetime. In this case it looks like it's fine, but also unnecessary. I'd personally get the string& instead and use that for later calls.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@411
PS13, Line 411: int ret = 0;
Initializing this up-front is a very old-school C idiom. Compilers can often optimize better if you declare the variable in the scope where it's used, so I'd prefer to move this to the else block at line 444 where it's actually needed.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/io/disk-io-mgr.cc@447
PS13, Line 447: ret = hdfsPreadFully(hdfs_conn, remote_hdfs_file, remote_file_offset,
If we wanted to get fancy we could use a lambda to return the read result. I think that adds no additional overhead, but also entirely unnecessary.
int ret = [&]() {
ScopedHistogramTimer read_timer(queue->read_latency());
return hdfsPreadFully(hdfs_conn, remote_hdfs_file, remote_file_offset, read_buffer_bloc->data(), local_file_size);
}();
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc
File be/src/runtime/io/scan-range.cc:
http://gerrit.cloudera.org:8080/#/c/17979/12/be/src/runtime/io/scan-range.cc@156
PS12, Line 156: // Set the correct reader to read the range if the memory buffer is not available.
> Added some comments. The logic here is to set the correct file reader if it
Ack
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr-test.cc
File be/src/runtime/tmp-file-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr-test.cc@2101
PS13, Line 2101: TEST_F(TmpFileMgrTest, TestBatchReadingFromRemote) {
Can we add a test case where the remote file is deleted while DoUpload is in progress, and verify it stops early? Not sure if that belongs here or in disk-io-mgr-test.cc.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1019
PS13, Line 1019: // For other status, will be treated as disabled.
So if MemBlock is reserved/allocated, that means another process has already called this function but the async RemoteOperRange hasn't finished yet? Might be nice to lay that out explicitly in the comment.
http://gerrit.cloudera.org:8080/#/c/17979/13/be/src/runtime/tmp-file-mgr.cc@1022
PS13, Line 1022: return;
Ok, took me a sec to see why these checks make sense even though the only caller - GetReadBufferFile - also does them before calling. These are done within the lock, so the caller is short-circuiting with unlocked calls to see if the memblock has advanced to written/disabled yet.
That would probably be clearer to me if all the checks were in this function rather than split across function and caller.
--
To view, visit http://gerrit.cloudera.org:8080/17979
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcc5d0881ffaeff09c5c514306cd668373ad31b
Gerrit-Change-Number: 17979
Gerrit-PatchSet: 13
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 28 Jun 2022 19:28:27 +0000
Gerrit-HasComments: Yes