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 Apache Kafka scaler config #5804

Merged

Conversation

wozniakjan
Copy link
Member

@wozniakjan wozniakjan commented May 15, 2024

Refactor Apache Kafka scaler config using declarative parser from #5676. This PR also introduces a new field tag option range. This tag helps with config options that don't want to enumerate all of the values but instead generate as range (or multiple ranges) bounded by from and to (or multiple from's and to's).

Example usage:

sc := &ScalerConfig{
TriggerMetadata: map[string]string{
"range": "5-10",
"multiRange": "5-10, 15-20",
"dottedRange": "2..7",
"wrongRange": "5..3",
},
}
type testStruct struct {
Range []int `keda:"name=range, order=triggerMetadata, rangeSeparator=-"`
MultiRange []int `keda:"name=multiRange, order=triggerMetadata, rangeSeparator=-"`
DottedRange []int `keda:"name=dottedRange, order=triggerMetadata, rangeSeparator=.."`
WrongRange []int `keda:"name=wrongRange, order=triggerMetadata, rangeSeparator=.."`
}

Checklist

Relates to #5797

@wozniakjan wozniakjan requested a review from a team as a code owner May 15, 2024 11:22
@wozniakjan wozniakjan force-pushed the refactor_apache_kafka_scaler_config branch from 69a2958 to 02fd305 Compare May 15, 2024 11:33
@wozniakjan
Copy link
Member Author

wozniakjan commented May 15, 2024

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

@wozniakjan wozniakjan marked this pull request as draft May 15, 2024 12:02
@dttung2905
Copy link
Contributor

@wozniakjan Oh I did not know you are working on apache_kafka_scaler.go. I'm working on the kafka_scaler.go but the progress is no where matching yours. Do you want to work on kafka_scaler.go too, since the code for both scalers are largely similar?

@wozniakjan
Copy link
Member Author

@wozniakjan Oh I did not know you are working on apache_kafka_scaler.go. I'm working on the kafka_scaler.go but the progress is no where matching yours. Do you want to work on kafka_scaler.go too, since the code for both scalers are largely similar?

my bad, I didn't realize they were similar and I was just going in alphabetical order one scaler at a time. Sure thing, and sorry for the inconvenience. Feel free to open WIP PR with what you have for the kafka_scaler.go and I can continue.

@wozniakjan wozniakjan changed the title Refactor Apache Kafka scaler config WIP: Refactor Apache Kafka scaler config May 16, 2024
@wozniakjan wozniakjan force-pushed the refactor_apache_kafka_scaler_config branch 2 times, most recently from caee424 to f905add Compare May 23, 2024 10:48
@wozniakjan
Copy link
Member Author

wozniakjan commented May 23, 2024

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

@wozniakjan wozniakjan changed the title WIP: Refactor Apache Kafka scaler config Refactor Apache Kafka scaler config May 23, 2024
@wozniakjan wozniakjan marked this pull request as ready for review May 23, 2024 11:11
@wozniakjan
Copy link
Member Author

ptal @kedacore/keda-contributors, @dttung2905, @SpiritZhou, not sure about the rangeSeparator, maybe I should just call it range and optionally take value for separator, otherwise assume - as default?

@zroubalik
Copy link
Member

ptal @kedacore/keda-contributors, @dttung2905, @SpiritZhou, not sure about the rangeSeparator, maybe I should just call it range and optionally take value for separator, otherwise assume - as default?

I like this

@wozniakjan wozniakjan force-pushed the refactor_apache_kafka_scaler_config branch from f905add to 621bdb9 Compare May 27, 2024 20:22
@wozniakjan
Copy link
Member Author

wozniakjan commented May 27, 2024

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

@wozniakjan wozniakjan force-pushed the refactor_apache_kafka_scaler_config branch from 621bdb9 to 8d9397c Compare May 28, 2024 08:39
@wozniakjan
Copy link
Member Author

wozniakjan commented May 28, 2024

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

@wozniakjan wozniakjan mentioned this pull request May 28, 2024
2 tasks
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
@wozniakjan wozniakjan force-pushed the refactor_apache_kafka_scaler_config branch from 8d9397c to f0b2949 Compare May 28, 2024 11:57
@wozniakjan
Copy link
Member Author

wozniakjan commented May 28, 2024

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

@wozniakjan
Copy link
Member Author

ptal @kedacore/keda-contributors

@zroubalik zroubalik merged commit 25b46f6 into kedacore:main May 29, 2024
20 checks passed
rxg8255 pushed a commit to rxg8255/keda that referenced this pull request Jun 24, 2024
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
Signed-off-by: Ranjith Gopal <ranjith.gopalreddy@7-11.com>
uucloud pushed a commit to uucloud/keda that referenced this pull request Jul 11, 2024
Signed-off-by: Jan Wozniak <wozniak.jan@gmail.com>
Signed-off-by: uucloud <uucloud@qq.com>
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.

3 participants