cache analyzer sources #401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pro-wh
reviewed
May 3, 2023
pro-wh
reviewed
May 3, 2023
cmd/analyzer/analyzer.go
Outdated
| target storage.TargetStorage | ||
| logger *log.Logger | ||
| } | ||
|
|
||
| type AllSources struct { |
There was a problem hiding this comment.
AbstractServiceClientFactoryAggregator here we come
There was a problem hiding this comment.
yeah... I was also considering SourceFactory but it's not quite satisfactory either
mitjat
approved these changes
May 3, 2023
cmd/analyzer/analyzer.go
Outdated
Comment on lines
245
to
255
| sdkPT := cfg.Source.SDKParaTime(common.RuntimeEmerald) | ||
| sourceClient, err1 := sources.Runtime(ctx, common.RuntimeEmerald) | ||
| if err1 != nil { | ||
| return nil, err1 | ||
| } |
There was a problem hiding this comment.
I'm a little unhappy about the code duplication here, but I don't have great suggestions.
Refloating a recent idea: If RuntimeClient used lazy initialization, we could inline sources.Runtime and not worry about errors. It would also make it so the query_on_cache_miss flag is not needed.
Definitely not something we'd do in this PR, and likely ever, just from the effort/benefits ratio. But a man can dream.
Andrew7234
force-pushed
the
andrew7234/unify-analyzer-source
branch
from
d6a7fd4 to
8bd1aaa
Compare
tweaking var names linter do not close source in analyzers; rename vars; update comments
Andrew7234
force-pushed
the
andrew7234/unify-analyzer-source
branch
from
8bd1aaa to
4b75ba3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently pass the source config to each analyzer, and then the analyzer instantiates its own source. That's bad for two reasons:
Tasks:
todo: choose better var names