Skip to content

Refactor Apache Kafka scaler config#5804

Merged
zroubalik merged 2 commits into
kedacore:mainfrom
wozniakjan:refactor_apache_kafka_scaler_config
May 29, 2024
Merged

Refactor Apache Kafka scaler config#5804
zroubalik merged 2 commits into
kedacore:mainfrom
wozniakjan:refactor_apache_kafka_scaler_config

Conversation

@wozniakjan

@wozniakjan wozniakjan commented May 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

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:

keda/pkg/scalers/scalersconfig/typed_config_test.go

Lines 523 to 537 in f905add

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

wozniakjan commented May 15, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member Author

/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

dttung2905 commented May 15, 2024

Copy link
Copy Markdown
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?

@dttung2905 dttung2905 mentioned this pull request May 15, 2024
Closed
58 tasks
@wozniakjan

wozniakjan commented May 16, 2024

Copy link
Copy Markdown
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
wozniakjan commented May 23, 2024
View reviewed changes
Comment thread pkg/scalers/apache_kafka_scaler.go
@wozniakjan

wozniakjan commented May 23, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member Author

/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

wozniakjan commented May 27, 2024

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

zroubalik commented May 27, 2024

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

wozniakjan commented May 27, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member Author

/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

wozniakjan commented May 28, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member Author

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

@wozniakjan wozniakjan mentioned this pull request May 28, 2024
Merged
2 tasks
@wozniakjan wozniakjan force-pushed the refactor_apache_kafka_scaler_config branch from 8d9397c to f0b2949 Compare May 28, 2024 11:57
@wozniakjan

wozniakjan commented May 28, 2024 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member Author

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

@wozniakjan

wozniakjan commented May 29, 2024

Copy link
Copy Markdown
Member Author

ptal @kedacore/keda-contributors

@zroubalik zroubalik merged commit 25b46f6 into kedacore:main May 29, 2024
rxg8255 pushed a commit to rxg8255/keda that referenced this pull request Jun 24, 2024
@wozniakjan
Refactor Apache Kafka scaler config (kedacore#5804)
fff7eac 
Signed-off-by: Jan Wozniak <[email protected]>
Signed-off-by: Ranjith Gopal <[email protected]>
uucloud pushed a commit to uucloud/keda that referenced this pull request Jul 11, 2024
@wozniakjan @uucloud
Refactor Apache Kafka scaler config (kedacore#5804)
2a70de9 
Signed-off-by: Jan Wozniak <[email protected]>
Signed-off-by: uucloud <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zroubalik zroubalik zroubalik approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wozniakjan @dttung2905 @zroubalik