Adding support for influx v3 in the influx scaler #5513
Conversation
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
| case config.TriggerMetadata["influxVersion"] == "3": | ||
| organizationName = val |
There was a problem hiding this comment.
Is this correct? I mean, in this case val is "". If this is intender, I'd prefer to make it explicit
There was a problem hiding this comment.
organizations are not used in influx v3 (only databases) so its currently ignored if passed when using that version. Would it be better to use organizationName = "" here to make it more explicit (and keep it ignored) or throw an error if it's passed when using v3 with "organization not supported for Influx v3"?
There was a problem hiding this comment.
happy to take any path here, just let me know
|
Something important to note - I do not have an influx v2 deployment available to perform any regression testing for that version. I have verified scaling a deployment using influx v3 though. |
I'll take a look at these |
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
This isn't going to be possible as there's no public build for v3 available yet. It will be at some point later this year. |
pkg/scalers/influxdb_scaler.go
Outdated
| }) | ||
|
|
||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
updated, please let me know if it looks ok
pkg/scalers/influxdb_scaler.go
Outdated
| @@ -47,7 +61,29 @@ func NewInfluxDBScaler(config *scalersconfig.ScalerConfig) (Scaler, error) { | |||
| return nil, fmt.Errorf("error parsing influxdb metadata: %w", err) | |||
| } | |||
|
|
|||
| logger.Info("starting up influxdb client") | |||
| if meta.influxVersion == "3" { | |||
| logger.Info("starting up influxdb v3 client") | |||
There was a problem hiding this comment.
this would be to verbose, let add V(1) here
pkg/scalers/influxdb_scaler.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| logger.Info("starting up influxdb v2 client") |
There was a problem hiding this comment.
this would be to verbose, let add V(1) here
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
Signed-off-by: Daniel Whatmuff <danielwhatmuff@gmail.com>
yeah, we should |
|
@zroubalik @JorTurFer Unfortunately, influxdata have yet to provide public images for influxdb v3. They've claimed this is eventually coming, both to us privately as customers and publicly here: https://www.influxdata.com/blog/the-plan-for-influxdb-3-0-open-source/. I don't think E2E testing is possible until this becomes reality, correct? If it means anything, we've been running a version of Keda with these changes in production against our commercial Influxdb V3 installation for a while now and it's been stable. Is there a path to get this merged without E2E tests against v3, possibly marking support as experimental, as long as the v2 tests continue to pass? |
We shouldn't merge any feature without the proper e2e tests. As the core is already public, can we just build our own image and use it temporally until they release their own one? |
Fixes #5445
Relates to kedacore/keda-docs#1318