[storage] Remove usages of jaegerstorage.GetStorageFactory #6624
Conversation
Signed-off-by: Aryan Goyal <[email protected]>
|
Thought of writing a draft PR to help move this issue forward, Hope that's okay?
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6624 +/- ##
==========================================
- Coverage 95.93% 95.78% -0.16%
==========================================
Files 365 365
Lines 20616 20637 +21
==========================================
- Hits 19778 19767 -11
- Misses 638 667 +29
- Partials 200 203 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
If a PR claims to resolve "Remove usages ..." issue then I expect the actual |
|
I wanted to validate my understanding for this issue first i.e., if implementing storage_v1 interfaces in the v1 factory adapter is the correct approach. Edit: |
| @@ -61,3 +64,33 @@ func (f *Factory) CreateDependencyReader() (depstore.Reader, error) { | |||
| } | |||
| return NewDependencyReader(dr), nil | |||
| } | |||
|
|
|||
| // CreateLock implements storage_v1.SamplingStoreFactory | |||
| func (f *Factory) CreateLock() (distributedlock.Lock, error) { | |||
There was a problem hiding this comment.
@yurishkuro This approach works but there's a slight change in behaviour. Consider the following:
- Before this PR, if a storage didn't implement the
storage.SamplingStoreFactory- it would fail here - As a result of the changes PR, the check I linked above will always pass but it would fail in the following check here
I don't see a huge downside to doing it this way except maybe a slightly different/confusing error message. We could mitigate that by returning a specific error here that we check for upstream and return the same error message as before. What do you think? The other approach would be to only construct the factory adapter based on the interfaces the underlying factory implements.
There was a problem hiding this comment.
As long as we fail during startup, as opposed to later when processing incoming data, it should be fine.
There was a problem hiding this comment.
We could mitigate that by returning a specific error here that we check for upstream and return the same error message as before
I've changed the error checking to something along the lines of this. Please have a look.
Signed-off-by: Aryan Goyal <[email protected]>
Signed-off-by: Aryan Goyal <[email protected]>
…into remove-getstoragefactory-usage
| @@ -55,7 +56,11 @@ func (c *storageCleaner) Start(_ context.Context, host component.Host) error { | |||
| if !ok { | |||
| return fmt.Errorf("storage %s does not implement Purger interface", c.config.TraceStorage) | |||
| } | |||
| if err := purger.Purge(httpContext); err != nil { | |||
| err = purger.Purge(httpContext) | |||
| if errors.Is(err, v1adapter.ErrPurgerNotImplemented) { | |||
There was a problem hiding this comment.
it doesn't make sense to be checking an error from v1adapter here. We need storage_v2 APIs to be self-sufficient. I would recommend moving Sampling/Lock/Purger APIs to storage_v2 and replacing them with type aliases in v1 storage (it should be a separate PR before this one).
There was a problem hiding this comment.
I'm a bit confused, wouldn't we still need to check for the casting error in the adapter to return an error like storage %s does not implement Purger interface with the correct storage_name that is used?
I would recommend moving Sampling/Lock/Purger APIs to storage_v2 and replacing them with type aliases in v1 storage (it should be a separate PR before this one)
Should each of these APIs be in a different module in storage_v2, in a similar way DependencyStore has been moved to storage_v2/depstore? Or should they be defined in the storage_v2/tracestore? What do you think?
Which problem is this PR solving?
GetStorageFactory#6564Description of the changes
How was this change tested?
make testChecklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test