Skip to content
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

Keda Pulsar integration doc says msgBacklogThreshold is the parameter but code is looking for msgBacklog #4681

Closed
Tracked by #4795
frankjkelly opened this issue Jun 12, 2023 · 7 comments · Fixed by #4736
Labels
bug Something isn't working good first issue Good for newcomers help wanted Looking for support from community

Comments

@frankjkelly
Copy link

frankjkelly commented Jun 12, 2023

Report

Compare https://keda.sh/docs/2.10/scalers/pulsar/
mentioning
msgBacklogThreshold: '5'
to

msgBacklogMetricName = "msgBacklog"

Expected Behavior

Code matches documentation

Actual Behavior

Code does not match documentation - overriding msgBacklogThreshold has no impact

Steps to Reproduce the Problem

  1. Compare Docs and code

Logs from KEDA operator

N/A

KEDA Version

2.10.1

Kubernetes Version

< 1.23

Platform

Amazon Web Services

Scaler Details

Pulsar

Anything else?

No response

@frankjkelly frankjkelly added the bug Something isn't working label Jun 12, 2023
@JorTurFer
Copy link
Member

Wow, nice catch!
Other scalers use the suffix "threshold", so I'd update the code... Are you willing to open a PR fixing it? Even if it's not documented, we should maintain both IMO as it's something crucial for the scaler.
I reckon that just updating this code with this could be enough (and adding a tests case):

	// FIXME: msgBacklog support DEPRECATED to be removed in v2.13
	if val, ok := config.TriggerMetadata[msgBacklogMetricName]; ok {
		logger.Info("\"msgBacklog\" is deprecated and will be removed in v2.13, please use \"msgBacklogThreshold\" instead")
		t, err := strconv.ParseInt(val, 10, 64)
		if err != nil {
			return meta, fmt.Errorf("error parsing %s: %w", msgBacklogMetricName, err)
		}
		meta.msgBacklogThreshold = t
	} else if val, ok := config.TriggerMetadata["msgBacklogThreshold"]; ok {
		t, err := strconv.ParseInt(val, 10, 64)
		if err != nil {
			return meta, fmt.Errorf("error parsing %s: %w", msgBacklogMetricName, err)
		}
		meta.msgBacklogThreshold = t
	}
	// END FIXME

I have reduced the code to include a snippet, but the logger should be propagated until here too

@frankjkelly
Copy link
Author

@JorTurFer Sorry I'm not great with Golang - if there were already some test cases with msgBacklogThreshold I could probably wing it.

@JorTurFer
Copy link
Member

These are unit test for pulsar: https://github.com/kedacore/keda/blob/main/pkg/scalers/pulsar_scaler_test.go
I have quickly checked and there isn't any test checking that (I guess that's been the problem) but it shouldn't be too difficult for being added.
If you don't feel comfortable with golang, don't worry. Let's keep this issue open and wait for community help

@JorTurFer JorTurFer added help wanted Looking for support from community good first issue Good for newcomers labels Jun 12, 2023
@Mutusva
Copy link

Mutusva commented Jun 15, 2023

I am starting out and would like to contribute. I have knowledge of GO is that fine if I open the PR.

@JorTurFer
Copy link
Member

Hey!
sure @Mutusva , it'd be nice ❤️

@dttung2905
Copy link
Contributor

Hi @Mutusva, are you still working on this ? I can author a quick PR if you don't mind

@Mutusva
Copy link

Mutusva commented Jun 25, 2023

Hi @Mutusva, are you still working on this ? I can author a quick PR if you don't mind

Hi @dttung2905 sure you can take it up, had travelled and my internet isn't so stable

@JorTurFer JorTurFer mentioned this issue Jul 14, 2023
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Looking for support from community
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants