SOLR-17150: Create MemAllowedLimit #2708
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
documentation
gus-asf
requested changes
Sep 25, 2024
solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc
Outdated
Show resolved
Hide resolved
solr/solr-ref-guide/modules/query-guide/pages/common-query-parameters.adoc
Outdated
Show resolved
Hide resolved
gus-asf
requested changes
Oct 7, 2024
| * Enforces a memory-based limit on a given SolrQueryRequest, as specified by the {@code memAllowed} | ||
| * query parameter. | ||
| * | ||
| * <p>This class tracks per-thread memory allocations using its own ThreadLocal. It records the |
There was a problem hiding this comment.
"per-thread memory allocations during a request using it's own Thread Local."
I think that phrase would give the gist without the reader needing to parse the following logic.
| initAndGetCurrentAllocatedBytes(); | ||
| } | ||
|
|
||
| private long initAndGetCurrentAllocatedBytes() { |
There was a problem hiding this comment.
Methods called in constructors should be final.
There was a problem hiding this comment.
This method is private, nothing can override it.
| @@ -99,9 +136,8 @@ public boolean shouldExit() { | |||
| } | |||
|
|
|||
| try { | |||
| long currentAllocatedBytes = (Long) GET_BYTES_METHOD.invoke(threadBean); | |||
| long currentAllocatedBytes = initAndGetCurrentAllocatedBytes(); | |||
There was a problem hiding this comment.
I feel like you should have an init() method (or just code in the constructor), and a getCurrentAllocatedBytes() method and thus avoid all init logic in subsequent calls? I believe Java guarantees that nothing calls methods on an object until the constructor completes.
gus-asf
approved these changes
Oct 7, 2024
sigram
added a commit
to sigram/solr
that referenced
this pull request
Oct 7, 2024
sigram
added a commit
that referenced
this pull request
Oct 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refreshed PR using the latest QueryLimit APIs and a thread-safe accumulation of value, similar to the approach in
CpuAllowedLimit. However, since that class uses a thread-local managed byThreadCpuTimerthis implementation adds its own thread-local.