Remove archive storage factory #5279
Conversation
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
| return false | ||
| } | ||
| reader, err := archiveFactory.CreateArchiveSpanReader() | ||
| reader, err := storageFactory.CreateSpanReader() |
There was a problem hiding this comment.
This is not equivalent to previous behavior. Whoever calls this function is passing the primary storage factory, and you're just using it straight up.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5279 +/- ##
===========================================
- Coverage 95.16% 41.01% -54.15%
===========================================
Files 340 212 -128
Lines 16666 10981 -5685
===========================================
- Hits 15860 4504 -11356
- Misses 616 6073 +5457
- Partials 190 404 +214
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
storage/factory.go
Outdated
| @@ -45,6 +45,12 @@ type Factory interface { | |||
| // CreateSpanWriter creates a spanstore.Writer. | |||
| CreateSpanWriter() (spanstore.Writer, error) | |||
|
|
|||
| // CreateArchiveSpanReader creates a spanstore.Writer for archived spans. | |||
| CreateArchiveSpanReader() (spanstore.Reader, error) | |||
There was a problem hiding this comment.
I think you are on the wrong path. The point was to remove "archive" from the interfaces (since its interface is identical to span storage), and only keep it as a configuration option. Eg
var primary = MyStorage(priArgs)
var archive = MyStorage(archArgs)
Here it's using the same api and implementation, just calls it "archive".
If at all possible we should try to avoid changing anything in v1 storage interface, because it's not necessary for v2. What does need to change is how query-service obtains an instance of archive storage. Right now query-service gets only one storage factory, and tries to cast it to ArchiveStorageFactory. Instead we should find a way to give it two factories, separately for primary and archive storage (if needed we can cast in main). Then for v2 it would be trivial because it naturally gets two independent factories already labeled primary/archive and it can pass them to the query-service.
|
@akagami-harsh I see you fixing tests, but the direction itself doesn't seem right to me (#5279 (comment)). Can you elaborate? |
@yurishkuro , i am trying do to what you explained in the comment. first i removed the storage.archiveFactory interface and then trying to find a way to pass in two separate factories in the query-service for primary and archive. |
|
I also said we should only do it in the scope of jaeger-v2, not do a major change in v1 storage
|
Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
|
I think we can close this |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test