Refactor everywhere uses ESClient to have a Switch #6168
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
idls
Outdated
There was a problem hiding this comment.
idls updated by mistake?
There was a problem hiding this comment.
Yes. Trying to remove those
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| case ES: | ||
| err = w.emitWorkflowTypeCountMetricsES(ctx, domainName, logger) | ||
| default: | ||
| err = w.emitWorkflowTypeCountMetricsES(ctx, domainName, logger) |
There was a problem hiding this comment.
both cases doing to same thing. did you mean to emit a metric for pinot here
There was a problem hiding this comment.
Yes. I'll add pinot in the next PR
| @@ -283,6 +283,7 @@ func (s *Service) startESAnalyzer() { | |||
| s.GetFrontendClient(), | |||
| s.GetClientBean(), | |||
| s.params.ESClient, | |||
| s.params.PinotClient, | |||
There was a problem hiding this comment.
nit: it's fine for this PR but as a general pattern we should avoid passing multiple client objects to a component and having it decide based on which one is nil. Ideally there would be one parameter here of visibilityClient type or something like that and the bootstrap code (this file) determines which one to pass there.
| var mode readMode | ||
| if esClient != nil { | ||
| mode = ES | ||
| } else if pinotClient != nil { | ||
| mode = Pinot | ||
| } |
There was a problem hiding this comment.
since this prefers ES if both are present:
could this mode be replaced by "just use the non-nil client" and only pass one?
or is there more stuff coming that'll use both sometimes?
There was a problem hiding this comment.
Yes, it can be. It's just that Pinot and ES has different GenericClients, there'll need a lot efforts to do a refactor.
There was a problem hiding this comment.
aaah, because esClient is used by other things too, and there is / will be shadowing, so these fields are not just for this mode.
alrighty, makes sense 👍
What changed?
Refactor everywhere uses ESClient to have a switch to deal with different clients
Also added more unit tests
Why?
We need to add Pinot client in this analyzer to make it a VisibilityAnalyzer instead of an ESAnalzyer.
How did you test it?
Unit test
Potential risks
Release notes
Documentation Changes