-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(badger): Add missing SamplingStoreFactory.CreateLock method #4966
fix(badger): Add missing SamplingStoreFactory.CreateLock method #4966
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4966 +/- ##
==========================================
+ Coverage 95.59% 95.60% +0.01%
==========================================
Files 317 318 +1
Lines 18689 18695 +6
==========================================
+ Hits 17866 17874 +8
+ Misses 661 659 -2
Partials 162 162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@slayer321 this issue should've been caught at compile time, and via integration test, but it looks like this PR is not changing any of those. The storage factory must implement this interface Line 54 in 47f39a8
I think the issue with integration tests is that the StorageIntegration struct is defined with direct access to SpanWriter etc. that are pre-initialized by the concrete implementation. Instead it would be better if implementations would only initialize the respective factories and let the test retrieve writer, etc. (we could do it in another PR). |
Cf: #4967 |
…r badger Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
206224c
to
280cdec
Compare
Signed-off-by: slayer321 <sachin.maurya7666@gmail.com>
Thanks! |
Which problem is this PR solving?
Description of the changes
CreateLock
method onSamplingStoreFactory
interface for badgerHow was this change tested?
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test