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/02/12 10:47:37 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #10884: Reload segment usage when starting the process
abhishekagarwal87 opened a new pull request #10884:
URL: https://github.com/apache/druid/pull/10884
### Description
When a historical process is restarted, we lose the accounting being kept by the `StorageLocation` class. This implies that `StorageLocation` could reserve space for a segment even if the cache is completely full. Since after restart, `StorageLocation` thinks that current usage is zero.
<hr>
This PR has:
- [ ] 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 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/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] 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.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
* `SegmentLoaderLocalCacheManager`
* `StorageLocation`
* `SegmentLoadDropHandlerCacheTest`
----------------------------------------------------------------
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.
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] abhishekagarwal87 commented on pull request #10884: Reload segment usage when starting the process
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10884:
URL: https://github.com/apache/druid/pull/10884#issuecomment-783131486
Travis failure is unrelated.
----------------------------------------------------------------
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.
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] jon-wei commented on a change in pull request #10884: Reload segment usage when starting the process
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #10884:
URL: https://github.com/apache/druid/pull/10884#discussion_r578146685
##########
File path: server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java
##########
@@ -117,6 +117,33 @@ public synchronized File reserve(String segmentDir, DataSegment segment)
return reserve(segmentDir, segment.getId().toString(), segment.getSize());
}
+ /**
+ * Reserves space to store the given segment, only if it has not been done already. This can be used
+ * when segment is already downloaded on the disk. Unlike {@link #reserve(String, DataSegment)}, this function
+ * skips the check on disk availability. We also account for segment usage even if available size dips below 0.
+ * Such a situation indicates a configuration problem or a bug and we don't let segment loading fail because
+ * of this.
+ */
+ public synchronized void maybeReserve(String segmentFilePathToAdd, DataSegment segment)
+ {
+ final File segmentFileToAdd = new File(path, segmentFilePathToAdd);
+ if (files.contains(segmentFileToAdd)) {
+ // Already reserved
+ return;
+ }
+ files.add(segmentFileToAdd);
+ currSizeBytes += segment.getSize();
+ if (availableSizeBytes() < 0) {
+ log.error(
Review comment:
It might be worth upgrading the log level to alert for this, since I think it'd be a good idea to take prompt action when this happens.
What kind of configuration problems do you foresee leading to this error (would it happen if a user drops the maxSize on a historical with a filled segment cache? are there other cases?)
It might be worth more details on the corrective action in the log as well (like if the user should increase or decrease the parameter)
----------------------------------------------------------------
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.
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] abhishekagarwal87 commented on a change in pull request #10884: Reload segment usage when starting the process
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10884:
URL: https://github.com/apache/druid/pull/10884#discussion_r578269049
##########
File path: server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java
##########
@@ -117,6 +117,33 @@ public synchronized File reserve(String segmentDir, DataSegment segment)
return reserve(segmentDir, segment.getId().toString(), segment.getSize());
}
+ /**
+ * Reserves space to store the given segment, only if it has not been done already. This can be used
+ * when segment is already downloaded on the disk. Unlike {@link #reserve(String, DataSegment)}, this function
+ * skips the check on disk availability. We also account for segment usage even if available size dips below 0.
+ * Such a situation indicates a configuration problem or a bug and we don't let segment loading fail because
+ * of this.
+ */
+ public synchronized void maybeReserve(String segmentFilePathToAdd, DataSegment segment)
+ {
+ final File segmentFileToAdd = new File(path, segmentFilePathToAdd);
+ if (files.contains(segmentFileToAdd)) {
+ // Already reserved
+ return;
+ }
+ files.add(segmentFileToAdd);
+ currSizeBytes += segment.getSize();
+ if (availableSizeBytes() < 0) {
+ log.error(
Review comment:
> It might be worth upgrading the log level to alert for this, since I think it'd be a good idea to take prompt action when this happens.
Makes sense.
> What kind of configuration problems do you foresee leading to this error (would it happen if a user drops the maxSize on a historical with a filled segment cache? are there other cases?)
I don't expect this to happen unless someone reduced the max size or you re-attached a different data disk to the node. Most likely, if it happens, it's a bug somewhere in the code.
----------------------------------------------------------------
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.
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] abhishekagarwal87 commented on a change in pull request #10884: Reload segment usage when starting the process
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10884:
URL: https://github.com/apache/druid/pull/10884#discussion_r578269049
##########
File path: server/src/main/java/org/apache/druid/segment/loading/StorageLocation.java
##########
@@ -117,6 +117,33 @@ public synchronized File reserve(String segmentDir, DataSegment segment)
return reserve(segmentDir, segment.getId().toString(), segment.getSize());
}
+ /**
+ * Reserves space to store the given segment, only if it has not been done already. This can be used
+ * when segment is already downloaded on the disk. Unlike {@link #reserve(String, DataSegment)}, this function
+ * skips the check on disk availability. We also account for segment usage even if available size dips below 0.
+ * Such a situation indicates a configuration problem or a bug and we don't let segment loading fail because
+ * of this.
+ */
+ public synchronized void maybeReserve(String segmentFilePathToAdd, DataSegment segment)
+ {
+ final File segmentFileToAdd = new File(path, segmentFilePathToAdd);
+ if (files.contains(segmentFileToAdd)) {
+ // Already reserved
+ return;
+ }
+ files.add(segmentFileToAdd);
+ currSizeBytes += segment.getSize();
+ if (availableSizeBytes() < 0) {
+ log.error(
Review comment:
> It might be worth upgrading the log level to alert for this, since I think it'd be a good idea to take prompt action when this happens.
Makes sense.
> What kind of configuration problems do you foresee leading to this error (would it happen if a user drops the maxSize on a historical with a filled segment cache? are there other cases?)
I don't expect this to happen unless someone reduced the max size or you re-attached a different data disk to the node. Most likely, if it happens, it's a bug somewhere in the code.
----------------------------------------------------------------
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.
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] maytasm merged pull request #10884: Reload segment usage when starting the process
Posted by GitBox <gi...@apache.org>.
maytasm merged pull request #10884:
URL: https://github.com/apache/druid/pull/10884
----------------------------------------------------------------
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.
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