-
Notifications
You must be signed in to change notification settings - Fork 515
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: reset metadata fetcher init error after successful sync #10360
fix: reset metadata fetcher init error after successful sync #10360
Conversation
This pull request does not have a backport label. Could you fix it @kruskall? 🙏
NOTE: |
📚 Go benchmark reportDiff with the
report generated with https://pkg.go.dev/golang.org/x/perf/cmd/benchstat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an improvement, but I still think we can do better.
Can you please respond to my comment here? #10338 (comment)
What is the benefit of the initial ping? Why do we block in SourcemapFetcher.Fetch
? Would it be a problem if we just returned ErrUnavailable
until the metadata cache is populated? As it is, there's a window where SourcemapFetcher.Fetch
may behave as if the cache is empty, after the initial ping succeeds but before the sync
completes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think we should probably remove initErr
in #10367 though, and only mark the metadata fetcher as ready when it has successfully synced. Any errors would be logged, but not returned to the SourcemapFetcher.
@Mergifyio backport 8.7 |
✅ Backports have been created
|
(cherry picked from commit f3e8719)
Motivation/summary
From the related issue:
Checklist
apmpackage
have been made)For functional changes, consider:
How to test these changes
Related issues
Related to #10338