Fix the race condition of reflection scanning classes #9167
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
walterddr
approved these changes
Aug 4, 2022
| } | ||
| } | ||
|
|
||
| /** | ||
| * Due to the multi-threading issue in org.reflections.vfs.ZipDir, we need to put a lock before calling the | ||
| * reflection related methods. | ||
| * | ||
| * @return | ||
| */ | ||
| public static Object getReflectionLock() { |
There was a problem hiding this comment.
This is no longer needed i think or change the synchronize block above to also acquire via this method?
There was a problem hiding this comment.
This is needed to wrap the setup of swagger. Added runWithLock() as @klsince suggested, and deprecated this one
klsince
reviewed
Aug 4, 2022
| } | ||
| } | ||
|
|
||
| /** | ||
| * Due to the multi-threading issue in org.reflections.vfs.ZipDir, we need to put a lock before calling the | ||
| * reflection related methods. | ||
| * | ||
| * @return | ||
| */ | ||
| public static Object getReflectionLock() { |
There was a problem hiding this comment.
nit: instead of return lock, may do
runWithLock(Runnable r) {
sync(lock) {
r.run()
}
}
Jackie-Jiang
force-pushed
the
fix_reflections_race_condition
branch
from
1882325 to
ba9d4e4
Compare
klsince
approved these changes
Aug 5, 2022
Codecov Report
@@ Coverage Diff @@
## master #9167 +/- ##
============================================
- Coverage 69.99% 69.97% -0.03%
- Complexity 4682 4757 +75
============================================
Files 1848 1847 -1
Lines 98572 98541 -31
Branches 14967 14963 -4
============================================
- Hits 68999 68951 -48
- Misses 24723 24753 +30
+ Partials 4850 4837 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
siddharthteotia
approved these changes
Aug 5, 2022
|
we need to cherry-pick this back to release 0.11 branch |
walterddr
pushed a commit
to walterddr/pinot
that referenced
this pull request
Aug 5, 2022
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.
In #5446, we introduced reflection lock to solve the race condition of reflection library scanning the same jar from multiple threads (more details can be found in #5531).
Some new added reflection-based modules (
SegmentLoader,Tuner) didn't protect the reflection with lock. This PR enhances thePinotReflectionUtilsto support more use cases, and move the new modules to use it.