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 2024/01/04 07:22:50 UTC
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Yida Wu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20852
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partial written temporary files
The patch fixes a bug that the partial written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() during the destruction of the
LocalFileWriter class, ensuring proper release of the file handle.
Additionally, the patch introduces the immediate release of the
file handle by adding the file writer reset when the Delete()
is called within the DiskFile class.
Testing:
Manual testing of remote spilling, with the patch that there are
no references to the deleted files in the /proc/x/fd/. Conversely,
in the absence of the patch, such references are observed.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
3 files changed, 13 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/1
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10131/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 9
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 19:06:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 5:
The new patch fixes some test cases, and added a check in test_scratch_disk.py to automatically check the path. Also manually tested the case to cancel the query in the middle during spilling, everything looks fine, I don't see any deleted files in /proc/x/fd.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 14:20:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20852/1/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/20852/1/be/src/runtime/io/disk-file.cc@41
PS1, Line 41: // Close the file writer to release the file handle.
: RETURN_IF_ERROR(file_writer_->Close());
: }
> Is it safe to delete it here, so can we exclude the possibility of having p
Thanks Csaba! Yeah, it is better to call Close() here and add a DCHECK in the destructor of file writer. Changed.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 09:06:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14895/ : 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/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 12:37:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 6: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20852/6/tests/custom_cluster/test_scratch_disk.py
File tests/custom_cluster/test_scratch_disk.py:
http://gerrit.cloudera.org:8080/#/c/20852/6/tests/custom_cluster/test_scratch_disk.py@288
PS6, Line 288: result = subprocess.check_output(command, shell=True)
: return result.strip()
nit: -2 indentation
http://gerrit.cloudera.org:8080/#/c/20852/6/tests/custom_cluster/test_scratch_disk.py@291
PS6, Line 291: if not e.output:
: # If there is no output, return None.
: return None
: return "Error checking the fd path with error '{}'".format(e)
nit: -2 indentation
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 11:56:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 9
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 23:55:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 4: Code-Review+1
Would be good to also test some cases where spilling queries get killed either by user or fail for other reasons and in such cases do we cleanup resources properly? We could also inspect the error code path.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 18:02:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 4: Code-Review+2
it can be merged from my side - tests could be considered in a follow up Jira. Ideally Impala would never have hanging deleted files, so we could check after test runs that there are no FDs like this
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 10:48:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partially written temporary files
This patch fixes a bug where partially written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Passed core tests.
Additionally, a check has been added in the test
test_scratch_disk.py to verify that there are no deleted
files in the /proc/x/fd/ directory.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
M be/src/runtime/tmp-file-mgr-test.cc
M tests/custom_cluster/test_scratch_disk.py
6 files changed, 41 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/6
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14872/ : 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/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 09:27:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partial written temporary files
The patch fixes a bug that the partial written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Manual testing of remote spilling, configure a local path as the
local buffer and a S3 remote path for remote storage in scratch dir.
Ran tpcds q78 with a low buffer_pool_limit in the query option to
trigger spilling, and checked the path of local buffer path of the
scratch dir that the temporary files did exist during running the
query.
Without the fix, after running the query, we can find references
of deleted files in /proc/x/fd.
With the patch that there are no references to the deleted
files in the /proc/x/fd/.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
3 files changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/3
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20852/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20852/2//COMMIT_MSG@16
PS2, Line 16: Testing:
> can you add a bit more context here or in the Jira on how to trigger this?
The query that can spill (contains something like aggregation or hash join or others) should be good enough, in my test, I use tpcds q78, dont need to cancel, simply monitor the /proc/x/fd. Added some comments.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 12:34:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14890/ : 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/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 22:43:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 8: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/10130/
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 18:55:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/14869/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 07:49:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partially written temporary files
This patch fixes a bug where partially written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Manual testing of remote spilling, configure a local path as the
local buffer and a S3 remote path for remote storage in scratch dir.
Ran tpcds q78 with a low buffer_pool_limit in the query option to
trigger spilling, and checked the path of local buffer path of the
scratch dir that the temporary files did exist during running the
query.
Without the fix, after running the query, we can find references
of deleted files in /proc/x/fd.
With the patch that there are no references to the deleted
files in the /proc/x/fd/.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
3 files changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/4
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14889/ : 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/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 14:39:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partially written temporary files
This patch fixes a bug where partially written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Passed core tests.
Additionally, a check has been added in the test
test_scratch_disk.py to verify that there are no deleted
files in the /proc/x/fd/ directory.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Reviewed-on: http://gerrit.cloudera.org:8080/20852
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
M be/src/runtime/tmp-file-mgr-test.cc
M tests/custom_cluster/test_scratch_disk.py
6 files changed, 41 insertions(+), 4 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 10
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Wenzhe Zhou (Code Review)" <ge...@cloudera.org>.
Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 8:
hit IMPALA-11568
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 19:05:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 9
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 19:06:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20852/5/tests/custom_cluster/test_scratch_disk.py
File tests/custom_cluster/test_scratch_disk.py:
http://gerrit.cloudera.org:8080/#/c/20852/5/tests/custom_cluster/test_scratch_disk.py@300
PS5, Line 300: for impalad in self.cluster.impalads:
> You can get the list of impalads with
Thanks, this is better. Done.
http://gerrit.cloudera.org:8080/#/c/20852/5/tests/custom_cluster/test_scratch_disk.py@335
PS5, Line 335: @pytest.mark.execute_serially
> It would make sense to check this on each scratch test, not just on remote
Done
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 6
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 22:17:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partially written temporary files
This patch fixes a bug where partially written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Passed core tests.
Additionally, a check has been added in the test
test_scratch_disk.py to verify that there are no deleted
files in the /proc/x/fd/ directory.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
M be/src/runtime/tmp-file-mgr-test.cc
M tests/custom_cluster/test_scratch_disk.py
6 files changed, 50 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/5
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20852/1/be/src/runtime/io/disk-file.cc
File be/src/runtime/io/disk-file.cc:
http://gerrit.cloudera.org:8080/#/c/20852/1/be/src/runtime/io/disk-file.cc@41
PS1, Line 41: if (file_writer_ != nullptr) {
: file_writer_.reset();
: }
Is it safe to delete it here, so can we exclude the possibility of having pointers to file_writer_ elsewhere?
It seems safer to me to call Close here and possibly add a DCHECK to the destructor of LocalFileWriter to check that the file was closed.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 1
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 08:28:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20852/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20852/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-12681: Release file descriptors for partial written temporary files
nit: partially
http://gerrit.cloudera.org:8080/#/c/20852/3//COMMIT_MSG@9
PS3, Line 9: The patch fixes a bug that the partial written temporary files are
nit: This patch fixes a bug where partially written temporary files...
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 13:07:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partially written temporary files
This patch fixes a bug where partially written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Passed core tests.
Additionally, a check has been added in the test
test_scratch_disk.py to verify that there are no deleted
files in the /proc/x/fd/ directory.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
M be/src/runtime/tmp-file-mgr-test.cc
M tests/custom_cluster/test_scratch_disk.py
6 files changed, 41 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/7
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14873/ : 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/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 3
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 12:52:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14874/ : 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/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 15:14:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 4:
Thanks Csaba and Abhishek for the review. The patch still needs to change one or two tests before merge. Still testing, will upload a new patch soon.
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 11:42:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 14:17:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written 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/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/10130/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 8
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 14:17:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 14:04:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
IMPALA-12681: Release file descriptors for partial written temporary files
The patch fixes a bug that the partial written temporary files are
removed without releasing the file descriptors. This patch fixes
the bug by adding a call to Close() of the local file writer
during the Delete() of the DiskFile class, which could be called
when the local buffer file is being evicted or the query ends,
ensuring proper release of the file handle.
Testing:
Manual testing of remote spilling, with the patch that there are
no references to the deleted files in the /proc/x/fd/. Conversely,
in the absence of the patch, such references are observed.
Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
---
M be/src/runtime/io/disk-file.cc
M be/src/runtime/io/local-file-writer.cc
M be/src/runtime/io/local-file-writer.h
3 files changed, 11 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/20852/2
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partial written temporary files
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partial written temporary files
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20852/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20852/2//COMMIT_MSG@16
PS2, Line 16: Testing:
can you add a bit more context here or in the Jira on how to trigger this? e.g. what kind of query is needed, do we need to cancel the query....
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 2
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 11:11:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20852/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20852/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-12681: Release file descriptors for partially written temporary files
> nit: partially
Done
http://gerrit.cloudera.org:8080/#/c/20852/3//COMMIT_MSG@9
PS3, Line 9: This patch fixes a bug where partially written temporary files are
> nit: This patch fixes a bug where partially written temporary files...
Done
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 4
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Jan 2024 14:47:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20852/5/tests/custom_cluster/test_scratch_disk.py
File tests/custom_cluster/test_scratch_disk.py:
http://gerrit.cloudera.org:8080/#/c/20852/5/tests/custom_cluster/test_scratch_disk.py@300
PS5, Line 300: daemons = ['impalad']
You can get the list of impalads with
self.cluster.impalads
and get their pids with get_pid()
http://gerrit.cloudera.org:8080/#/c/20852/5/tests/custom_cluster/test_scratch_disk.py@335
PS5, Line 335: self.check_deleted_file_fd()
It would make sense to check this on each scratch test, not just on remote ones, e.g. by calling it in teardown_method
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 5
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Fri, 05 Jan 2024 15:55:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12681: Release file descriptors for partially written temporary files
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/20852 )
Change subject: IMPALA-12681: Release file descriptors for partially written temporary files
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20852/6/tests/custom_cluster/test_scratch_disk.py
File tests/custom_cluster/test_scratch_disk.py:
http://gerrit.cloudera.org:8080/#/c/20852/6/tests/custom_cluster/test_scratch_disk.py@288
PS6, Line 288: result = subprocess.check_output(command, shell=True)
: return result.strip()
> nit: -2 indentation
Done
http://gerrit.cloudera.org:8080/#/c/20852/6/tests/custom_cluster/test_scratch_disk.py@291
PS6, Line 291: if not e.output:
: # If there is no output, return None.
: return None
: return "Error checking the fd path with error '{}'".format(e)
> nit: -2 indentation
Done
--
To view, visit http://gerrit.cloudera.org:8080/20852
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I58a2bac419ced806d6f5a32bcdf24d79e078ab14
Gerrit-Change-Number: 20852
Gerrit-PatchSet: 7
Gerrit-Owner: Yida Wu <wy...@gmail.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Mon, 08 Jan 2024 12:10:20 +0000
Gerrit-HasComments: Yes