Fix es integration test 6094 #6157
Conversation
This change fixes two issues in ES integration tests: 1. Prevents index not found exceptions by proper index readiness checks 2. Prevents duplicate span detection issues The solution uses sync.Map for thread-safe: - Index existence caching - Span tracking Fixes jaegertracing#6094 Signed-off-by: ayush-gupta-dev <[email protected]>
This change fixes two issues in ES integration tests: 1. Prevents index not found exceptions by proper index readiness checks 2. Prevents duplicate span detection issues The solution uses sync.Map for thread-safe: - Index existence caching - Span tracking Fixes jaegertracing#6094 Signed-off-by: ayush-gupta-dev <[email protected]>
…test-6094 Signed-off-by: ayush-gupta-dev <[email protected]>
|
@yurishkuro, I hope you are well, will you review this when you have a moment? |
There was a problem hiding this comment.
I don't think this is the right solution or the right diagnosis of the problem. My hypothesis was that it is a race condition between creating templates and writing spans, not between creating indices and writing spans - our code never creates indices, it only creates index templates when factory is initialized.
| @@ -42,6 +44,63 @@ type SpanWriter struct { | |||
| serviceWriter serviceWriter | |||
| spanConverter dbmodel.FromDomain | |||
| spanServiceIndex spanAndServiceIndexFn | |||
| indexCache sync.Map | |||
| if _, exists := s.indexCache.Load(indexName); exists { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
the following statement looks like a CAS operation, so why do you need this one above?
| return fmt.Errorf("failed to check index existence: %w", err) | ||
| } | ||
|
|
||
| if !exists { |
There was a problem hiding this comment.
shortcircuit
if exists { return }
| if !exists { | ||
| s.logger.Info("Creating index", zap.String("index", indexName)) | ||
|
|
||
| // Set specific settings for the test environment |
There was a problem hiding this comment.
plugin/storage/es/spanstore/writer.go is production code, there's no place for test settings here
| } | ||
| }` | ||
|
|
||
| _, err = s.client().CreateIndex(indexName).Body(body).Do(ctx) |
There was a problem hiding this comment.
the writer should not be responsible for creating indices. The indices are created by ES automatically from templates on writes.
The templates are created by the factory:
jaeger/plugin/storage/es/factory.go
Line 279 in 61ad598
There was a problem hiding this comment.
I see, now i understand the core issue better:
- it's about template creation timing vs span writing.
- Indices are automatically created by ES based on templates.
- The race condition occurs in factory.go where templates are created and then spans are written before templates are fully ready?
Would this be a better approach?
- Remove all index management code from SpanWriter
- Add template creation synchronization in factory.go's createSpanWriter,
something like, passing context in createSpanWriter and in Create Templates function we can handle the context.. I am taling about these 2 functions, if thinking in correct direction shall i proceed and test?
There was a problem hiding this comment.
Visualizing like this (What do you think, shall i proceed with this?) I really think by passing context we can mititgate the issue.
Before:
Start -> Create Writer -> Start Template Creation -> Return Writer -> Write Spans (fails since template still creating)
[Templates Still Creating...]
After:
Start-> Create Writer -> Create Templates -> Retry if needed -> Templates Ready -> Return Writer -> Write Spans
There was a problem hiding this comment.
Yes, we want the sequence in the After part, we just need to enforce that the templates finished creating before returning the writer.
This change fixes two issues in ES integration tests:
The solution uses sync.Map for thread-safe:
Fixes #6094
Which problem is this PR solving?
Description of the changes
How was this change tested?