You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/10 13:56:38 UTC
[GitHub] [druid] loquisgon opened a new pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
loquisgon opened a new pull request #11903:
URL: https://github.com/apache/druid/pull/11903
Avoid materializing list of segment files (it can cause OOM/memory pressure) as well as looping over the files.
### Description
This fixes a memory pressure/OOM bug seen in the field when the middlemanager may get an "out of memory" error because the list of segments is too large. Besides, looping through the segments files is inefficient and this commit avoids that. It does not need new unit tests, the existing unit tests cover this code already.
This PR has:
- [ X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11903:
URL: https://github.com/apache/druid/pull/11903#issuecomment-965360290
This pull request **introduces 1 alert** when merging 94e419ed2fab597297ca55a27b81d37424841a74 into d3914c1a78ac4460665c910056b8976a31115483 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-e5ab83f1ded80224e23f91ab7ae527129ebc46f3)
**new alerts:**
* 1 for Uncontrolled data used in path expression
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #11903:
URL: https://github.com/apache/druid/pull/11903#discussion_r747111085
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/LocalIntermediaryDataManager.java
##########
@@ -377,20 +377,14 @@ public DataSegment addSegment(String supervisorTaskId, String subTaskId, DataSeg
final File partitionDir = new File(location.getPath(), getPartitionDirPath(supervisorTaskId, interval, bucketId));
if (partitionDir.exists()) {
supervisorTaskCheckTimes.put(supervisorTaskId, getExpiryTimeFromNow());
- final File[] segmentFiles = partitionDir.listFiles();
- if (segmentFiles == null) {
- return Optional.empty();
+ final File segmentFile = new File(partitionDir, subTaskId);
Review comment:
Thanks, fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon commented on a change in pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
Posted by GitBox <gi...@apache.org>.
loquisgon commented on a change in pull request #11903:
URL: https://github.com/apache/druid/pull/11903#discussion_r747690834
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/LocalIntermediaryDataManager.java
##########
@@ -377,20 +377,14 @@ public DataSegment addSegment(String supervisorTaskId, String subTaskId, DataSeg
final File partitionDir = new File(location.getPath(), getPartitionDirPath(supervisorTaskId, interval, bucketId));
if (partitionDir.exists()) {
supervisorTaskCheckTimes.put(supervisorTaskId, getExpiryTimeFromNow());
- final File[] segmentFiles = partitionDir.listFiles();
- if (segmentFiles == null) {
- return Optional.empty();
+ final File segmentFile = new File(partitionDir, subTaskId);
Review comment:
The new code is semantically the same, with the same branching (i.e. partition found it returns it else it returns Optional.empty() so I think we are good. The Optional.empty() return though is not being hit by any existing unit test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #11903:
URL: https://github.com/apache/druid/pull/11903#discussion_r746783612
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/LocalIntermediaryDataManager.java
##########
@@ -377,20 +377,14 @@ public DataSegment addSegment(String supervisorTaskId, String subTaskId, DataSeg
final File partitionDir = new File(location.getPath(), getPartitionDirPath(supervisorTaskId, interval, bucketId));
if (partitionDir.exists()) {
supervisorTaskCheckTimes.put(supervisorTaskId, getExpiryTimeFromNow());
- final File[] segmentFiles = partitionDir.listFiles();
- if (segmentFiles == null) {
- return Optional.empty();
+ final File segmentFile = new File(partitionDir, subTaskId);
Review comment:
LGTM is complaining because `subTaskId` is passed in from user input. A validation check similar to the one done for `supervisorTaskId` on line 375 should suppress the warning from LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #11903:
URL: https://github.com/apache/druid/pull/11903#discussion_r747697612
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/shuffle/LocalIntermediaryDataManager.java
##########
@@ -377,20 +377,14 @@ public DataSegment addSegment(String supervisorTaskId, String subTaskId, DataSeg
final File partitionDir = new File(location.getPath(), getPartitionDirPath(supervisorTaskId, interval, bucketId));
if (partitionDir.exists()) {
supervisorTaskCheckTimes.put(supervisorTaskId, getExpiryTimeFromNow());
- final File[] segmentFiles = partitionDir.listFiles();
- if (segmentFiles == null) {
- return Optional.empty();
+ final File segmentFile = new File(partitionDir, subTaskId);
Review comment:
It looks like the existing unit tests don't validate what happens when the segment file is missing.
<img width="1112" alt="Screen Shot 2021-11-11 at 9 38 30 AM" src="https://user-images.githubusercontent.com/44787917/141343789-f575c851-a810-4bb8-8cbc-8fc25a0faee4.png">
This logic is simple enough, and code coverage bot is happy, so I'm ok with not adding a test case for this if it seems like overkill
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] loquisgon merged pull request #11903: Avoid materializing list of segment files when finding a partition file during shuffle
Posted by GitBox <gi...@apache.org>.
loquisgon merged pull request #11903:
URL: https://github.com/apache/druid/pull/11903
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org