Refactor get config to `getParameterFromConfigV2` Kafka Scaler #5319

dttung2905 · 2023-12-28T15:50:41Z

Hi team,

I did the refactor for 1 scaler first to get your opinion on this. Chose Kafka because imo its the most complexed 😓 so its worth a try

Checklist

I have verified that my change is according to the deprecations & breaking changes policy
Changelog has been updated and is aligned with our changelog requirements
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Relates to #5037

github-actions · 2023-12-28T15:50:52Z

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Add an entry in our changelog in alphabetical order and link related issue
Update the documentation, if needed
Add unit & e2e tests for your changes
GitHub checks are passing
Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

Our contribution guide

pkg/scalers/kafka_scaler.go

wozniakjan

can you please add a changelog entry? I guess it should go in the other section as Kafka scaler: ...

wozniakjan · 2024-02-11T20:16:09Z

pkg/scalers/apache_kafka_scaler.go

+	tlsString, err := getParameterFromConfigV2(
+		config,
+		"tls",
+		reflect.TypeOf(""),
+		UseMetadata(true),
+		UseAuthentication(true),
+		UseResolvedEnv(false),
+		IsOptional(true),
+		WithDefaultVal("disable"),


nit: iirc, the default value for optional arg is false, so the UseResolvedEnv(false) could be omitted? and for brevity, can we omit the arguments?

would you consider something like this?

Suggested change

tlsString, err := getParameterFromConfigV2(

config,

"tls",

reflect.TypeOf(""),

UseMetadata(true),

UseAuthentication(true),

UseResolvedEnv(false),

IsOptional(true),

WithDefaultVal("disable"),

tlsString, err := getParameterFromConfigV2(

config,

"tls",

reflect.TypeOf(""),

UseMetadata(),

UseAuthentication(),

IsOptional(),

WithDefaultVal("disable"),

I think eleminating option function when it is not true make sense as we already initiate the default value in this

keda/pkg/scalers/scaler.go

Line 243 in ef46459

opt := &configOptions{defaultVal: ""}

As fo having the default value, I personally does not enjoy that as Golang does not support default arg value out of the box 😆 , although we can still make some changes to adopt this, for example
Before

func IsOptional(optional bool) Option { return func(opt *configOptions) { opt.isOptional = optional } }

After

func IsOptional(optionals ...bool) Option { optional := true // Default value if len(optionals) > 0 { optional = optionals[0] } return func(opt *configOptions) { opt.isOptional = optional } }

Let me know if you still want to go through with the second point on default args value

wozniakjan · 2024-02-19T07:55:21Z

I would personally just drop the argument from all optional helper functions

UseMetadata(true)
UseAuthentication(true)
IsOptional(true)
...

would become

UseMetadata()
UseAuthentication()
IsOptional()
...

the bool argument is imho not necessary and the absence of option is semantically equivalent to option(false).

dttung2905 · 2024-02-20T21:02:59Z

@JorTurFer Could you help to take a look at this when you have time ?

JorTurFer · 2024-02-23T20:28:02Z

Sorry, I've been travelling this week :(
Honestly, I love this new approach for parsing parameters. It unifies all the process and it's awesome, great job!

I agree with the suggestions from @wozniakjan , but you've already addressed them, so nothing else from my side 🥇

JorTurFer · 2024-02-23T20:28:10Z

/run-e2e kafka
Update: You can check the progress here

dttung2905 · 2024-02-24T17:43:53Z

let me take a look at the failed e2e tests

JorTurFer · 2024-02-24T19:05:01Z

/run-e2e kafka
Update: You can check the progress here

dttung2905 · 2024-02-24T22:48:14Z

Seems like the similar failures is happening on the daily E2E test on main branch too link

The current failed E2E test shows similar pattern ( not exactly the same test case but still within the apache kafka scaler test )
The weird thing is when I try to reproduce the E2E tests on my local machine, it is showing time out

 apache_kafka_test.go:610: --- testing  limitToPartitionsWithLag: no scale out ---
    helper.go:548: Applying template: singleDeploymentTemplate
    helper.go:548: Applying template: limitToPartionsWithLagScaledObjectTemplate
    helper.go:500: Waiting for some time to ensure deployment replica count doesn't change from 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:500: Waiting for some time to ensure deployment replica count doesn't change from 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
    helper.go:507: Deployment - apache-kafka-test-deployment, Current  - 0
panic: test timed out after 10m0s
running tests:
	TestScaler (10m0s)

goroutine 1152 [running]:
testing.(*M).startAlarm.func1()
	/usr/local/go/src/testing/testing.go:2259 +0x3b9
created by time.goFunc
	/usr/local/go/src/time/sleep.go:176 +0x2d

goroutine 1 [chan receive, 10 minutes]:
testing.(*T).Run(0xc000304820, {0x186761b?, 0x52c5dc?}, 0x195caf8)
	/usr/local/go/src/testing/testing.go:1649 +0x3c8
testing.runTests.func1(0x26c6e00?)
	/usr/local/go/src/testing/testing.go:2054 +0x3e
testing.tRunner(0xc000304820, 0xc00019fc48)
	/usr/local/go/src/testing/testing.go:1595 +0xff
testing.runTests(0xc000238be0?, {0x26a0640, 0x1, 0x1}, {0x418a05?, 0xc00019fd08?, 0x26c5b20?})
	/usr/local/go/src/testing/testing.go:2052 +0x445
testing.(*M).Run(0xc000238be0)
	/usr/local/go/src/testing/testing.go:1925 +0x636
main.main()
	_testmain.go:47 +0x19c

The errors come from this line

keda/tests/scalers/apache_kafka/apache_kafka_test.go

Line 635 in b3f5548

    
           AssertReplicaCountNotChangeDuringTimePeriod(t, kc, deploymentName, testNamespace, 0, 30)

@JorTurFer Do you know how can we can increase the timeout to greater than 10m ? I tried to reduce the wait time of the AssertReplicaCountNotChangeDuringTimePeriod from 30 to 3 so the test passes that line but still throw a Timed out error down in the later section of the code

JorTurFer · 2024-03-30T16:07:51Z

Do you know how can we can increase the timeout to greater than 10m ?

Actually, the timeout is 20m:

keda/tests/run-all.go

Lines 27 to 33 in f2d86a8

    
           var ( 
        
           	concurrentTests        = 15 
        
           	regularTestsTimeout    = "20m" 
        
           	regularTestsRetries    = 3 
        
           	sequentialTestsTimeout = "20m" 
        
           	sequentialTestsRetries = 1 
        
           )

dttung2905 · 2024-04-01T19:41:49Z

let me rebase it . Hopefully the flaky test is gone from this PR #5610

JorTurFer · 2024-04-05T21:32:20Z

/run-e2e
Update: You can check the progress here

JorTurFer · 2024-04-06T00:01:04Z

/run-e2e kafka
Update: You can check the progress here

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Signed-off-by: Dao Thanh Tung <ttdao.2015@accountancy.smu.edu.sg>

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

dttung2905 · 2024-04-10T10:55:55Z

/run-e2e kafka
Update: You can check the progress here

zroubalik

@wozniakjan so are we gonna give this a try?

wozniakjan · 2024-04-10T17:30:03Z

I think there is great value in this PR, it's been reviewed, I vote for merge.

Parallel to this, I'm curious what the feedback will be on #5676, perhaps that approach can make the code even more readable. Or maybe the consensus will be that reflections and field tags are not that well suited for scaler configs and KEDA will continue with getParameterFromConfigV2

JorTurFer · 2024-04-10T20:45:29Z

hum... it's a fascinating approach that one which you're proposing tbh. It also can help decoupling the scaling metadata parsing from the scaler itself based on the tagging model and it's also really interesting IMHO.
WDYT @tomkerkhove @dttung2905 @zroubalik ?

dttung2905 · 2024-04-10T20:58:20Z

hum... it's a fascinating approach that one which you're proposing tbh. It also can help decoupling the scaling metadata parsing from the scaler itself based on the tagging model and it's also really interesting IMHO. WDYT @tomkerkhove @dttung2905 @zroubalik ?

I think this is an interesting approach from @wozniakjan , we should definitely give it a try. We can just keep this PR open as a reference in case we want to come back to this in the near future

wozniakjan · 2024-04-11T07:53:12Z

thank you, I will add #5676 to the agenda for the next KEDA community meeting, so we can discuss the advantages and disadvantages of the declarative config approach.

dttung2905 requested a review from a team as a code owner December 28, 2023 15:50

semgrep-app bot reviewed Dec 28, 2023

View reviewed changes

pkg/scalers/kafka_scaler.go Outdated Show resolved Hide resolved

dttung2905 changed the title ~~Refactor get config to getParameterFromConfigV2 Kafka Scaler~~ WIP:Refactor get config to getParameterFromConfigV2 Kafka Scaler Dec 28, 2023

zroubalik mentioned this pull request Jan 15, 2024

Release: 2.13.0 #5275

Closed

25 tasks

dttung2905 force-pushed the refactor-get-config-1 branch from fa091b2 to 8b8a4f2 Compare January 15, 2024 16:43

dttung2905 changed the title ~~WIP:Refactor get config to getParameterFromConfigV2 Kafka Scaler~~ Refactor get config to getParameterFromConfigV2 Kafka Scaler Jan 15, 2024

This was referenced Jan 16, 2024

[WIP] Generic parameter parser for scalers #5388

Closed

Refactor scalers parsing to unify them #5037

Open

dttung2905 mentioned this pull request Jan 16, 2024

Make improvement to getParameterFromConfigV2 #5391

Merged

4 tasks

wozniakjan reviewed Jan 30, 2024

View reviewed changes

dttung2905 force-pushed the refactor-get-config-1 branch from 8b8a4f2 to f6bd0fd Compare February 11, 2024 17:03

wozniakjan reviewed Feb 11, 2024

View reviewed changes

dttung2905 force-pushed the refactor-get-config-1 branch from f6bd0fd to ae7776b Compare February 19, 2024 20:14

JorTurFer approved these changes Feb 23, 2024

View reviewed changes

dttung2905 force-pushed the refactor-get-config-1 branch from 2bd0ae2 to 6158216 Compare April 1, 2024 19:47

dttung2905 mentioned this pull request Apr 5, 2024

Move to getParameterFromConfigV2 for input, excluding auth #5639

Open

7 tasks

dttung2905 force-pushed the refactor-get-config-1 branch from 6158216 to 466f0ed Compare April 5, 2024 21:13

First commit

978e2db

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

dttung2905 added 10 commits April 10, 2024 09:48

Rebase from master

2c9c21c

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Fix Semgrep

b088676

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Fix static check CI failure

51ad4a8

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Add apache_kafka_scaler

83416e2

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Finish apache kafka scaler

126de40

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Rebase from master

177d2b1

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

Rebase from master

24ad66c

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

refactor from review feedback

f250cc1

Signed-off-by: Dao Thanh Tung <ttdao.2015@accountancy.smu.edu.sg>

gofmt fix

40cde12

Signed-off-by: Dao Thanh Tung <ttdao.2015@accountancy.smu.edu.sg>

Fix minor typo

218c15f

Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>

dttung2905 force-pushed the refactor-get-config-1 branch from 466f0ed to 218c15f Compare April 10, 2024 09:28

wozniakjan mentioned this pull request Apr 10, 2024

Declarative scaler config #5676

Merged

4 tasks

zroubalik reviewed Apr 10, 2024

View reviewed changes

dttung2905 mentioned this pull request May 13, 2024

Refactor Scalers with new declarative scaler config #5797

Open

59 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor get config to `getParameterFromConfigV2` Kafka Scaler #5319

Refactor get config to `getParameterFromConfigV2` Kafka Scaler #5319

dttung2905 commented Dec 28, 2023

github-actions bot commented Dec 28, 2023

wozniakjan left a comment

wozniakjan Feb 11, 2024 •

edited

dttung2905 Feb 18, 2024

wozniakjan commented Feb 19, 2024

dttung2905 commented Feb 20, 2024

JorTurFer commented Feb 23, 2024

JorTurFer commented Feb 23, 2024 •

edited by github-actions bot

dttung2905 commented Feb 24, 2024

JorTurFer commented Feb 24, 2024 •

edited by github-actions bot

dttung2905 commented Feb 24, 2024

JorTurFer commented Mar 30, 2024

dttung2905 commented Apr 1, 2024

JorTurFer commented Apr 5, 2024 •

edited by github-actions bot

JorTurFer commented Apr 6, 2024 •

edited by github-actions bot

dttung2905 commented Apr 10, 2024 •

edited by github-actions bot

zroubalik left a comment

wozniakjan commented Apr 10, 2024 •

edited

JorTurFer commented Apr 10, 2024

dttung2905 commented Apr 10, 2024

wozniakjan commented Apr 11, 2024

Refactor get config to getParameterFromConfigV2 Kafka Scaler #5319

Are you sure you want to change the base?

Refactor get config to getParameterFromConfigV2 Kafka Scaler #5319

Conversation

dttung2905 commented Dec 28, 2023

Checklist

github-actions bot commented Dec 28, 2023

wozniakjan left a comment

Choose a reason for hiding this comment

wozniakjan Feb 11, 2024 • edited

Choose a reason for hiding this comment

dttung2905 Feb 18, 2024

Choose a reason for hiding this comment

wozniakjan commented Feb 19, 2024

dttung2905 commented Feb 20, 2024

JorTurFer commented Feb 23, 2024

JorTurFer commented Feb 23, 2024 • edited by github-actions bot

dttung2905 commented Feb 24, 2024

JorTurFer commented Feb 24, 2024 • edited by github-actions bot

dttung2905 commented Feb 24, 2024

JorTurFer commented Mar 30, 2024

dttung2905 commented Apr 1, 2024

JorTurFer commented Apr 5, 2024 • edited by github-actions bot

JorTurFer commented Apr 6, 2024 • edited by github-actions bot

dttung2905 commented Apr 10, 2024 • edited by github-actions bot

zroubalik left a comment

Choose a reason for hiding this comment

wozniakjan commented Apr 10, 2024 • edited

JorTurFer commented Apr 10, 2024

dttung2905 commented Apr 10, 2024

wozniakjan commented Apr 11, 2024

Refactor get config to `getParameterFromConfigV2` Kafka Scaler #5319

Refactor get config to `getParameterFromConfigV2` Kafka Scaler #5319

wozniakjan Feb 11, 2024 •

edited

JorTurFer commented Feb 23, 2024 •

edited by github-actions bot

JorTurFer commented Feb 24, 2024 •

edited by github-actions bot

JorTurFer commented Apr 5, 2024 •

edited by github-actions bot

JorTurFer commented Apr 6, 2024 •

edited by github-actions bot

dttung2905 commented Apr 10, 2024 •

edited by github-actions bot

wozniakjan commented Apr 10, 2024 •

edited