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

Refactor get config to getParameterFromConfigV2 Kafka Scaler #5319

Closed
wants to merge 11 commits into from

Conversation

dttung2905
Copy link
Contributor

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

Relates to #5037

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

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

While you are waiting, make sure to:

Learn more about:

pkg/scalers/kafka_scaler.go Outdated Show resolved Hide resolved
@dttung2905 dttung2905 changed the title Refactor get config to getParameterFromConfigV2 Kafka Scaler WIP:Refactor get config to getParameterFromConfigV2 Kafka Scaler Dec 28, 2023
@zroubalik zroubalik mentioned this pull request Jan 15, 2024
25 tasks
@dttung2905 dttung2905 changed the title WIP:Refactor get config to getParameterFromConfigV2 Kafka Scaler Refactor get config to getParameterFromConfigV2 Kafka Scaler Jan 15, 2024
Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 124 to 131
tlsString, err := getParameterFromConfigV2(
config,
"tls",
reflect.TypeOf(""),
UseMetadata(true),
UseAuthentication(true),
UseResolvedEnv(false),
IsOptional(true),
WithDefaultVal("disable"),
Copy link
Member

@wozniakjan wozniakjan Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Member

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
Copy link
Contributor Author

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

@JorTurFer
Copy link
Member

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
Copy link
Member

JorTurFer commented Feb 23, 2024

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

@dttung2905
Copy link
Contributor Author

let me take a look at the failed e2e tests

@JorTurFer
Copy link
Member

JorTurFer commented Feb 24, 2024

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

@dttung2905
Copy link
Contributor Author

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

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

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
Copy link
Member

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
Copy link
Contributor Author

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

@JorTurFer
Copy link
Member

JorTurFer commented Apr 5, 2024

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

@JorTurFer
Copy link
Member

JorTurFer commented Apr 6, 2024

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

Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
Signed-off-by: Dao Thanh Tung <[email protected]>
Signed-off-by: Dao Thanh Tung <[email protected]>
Signed-off-by: dttung2905 <[email protected]>
@dttung2905
Copy link
Contributor Author

dttung2905 commented Apr 10, 2024

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

@wozniakjan wozniakjan mentioned this pull request Apr 10, 2024
4 tasks
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wozniakjan so are we gonna give this a try?

@wozniakjan
Copy link
Member

wozniakjan commented Apr 10, 2024

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants