-
Notifications
You must be signed in to change notification settings - Fork 366
feat(trait): Auto-discover KEDA Kafka scaler from Camel URIs #6418
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
Conversation
squakez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work. The mapping is fine and we can add as much as we can support eventually. However, it seems we're missing to include this in the trait logic itself - did you forget to commit some file by any chance?
Then, some question. Here we're supporting the mapping of only a few parameters. How should we deal with additional parameters?
As for the structure of the code, I think it would be preferrable to have a pkg/trait/keda directory to have it better identified the resources. As for the mapping, it would be also likely convenient to have a ScaleMapper interface and a single scaler implementation for each. So, in /pkg/trait/keda/scalers we can have kakfa.go, sqs.go, ... having the specific logic of each self contained.
As a first POC we can keep it though, but we should add some documentation to clarify this is a WIP.
|
✔️ Unit test coverage report - coverage increased from 51.6% to 51.7% (+0.1%) |
|
@squakez Thanks for the initial review. Yes some files did not get committed. Even I was thinking there is a certain flaw with my current implementation -
What do you think about this merged approach ? Secondly regarding structure of the code: Definitely can work on a better interface based approach as you suggested with |
|
✔️ Unit test coverage report - coverage increased from 51.6% to 51.7% (+0.1%) |
|
@pkalsi97 yeah, I think the approach you've proposed is more consistent. Also the structure is exactly what I was thinking. When you continue with the development, my suggestion is that you start with the development of a single mapper (ideally, kafka). It's better to have a strong and complete reference, and we can later add more mappers on demand. |
|
@squakez sure. Thanks for the input! |
|
@squakez I have refactored the implementation as suggested, also updated the PR description and title. Thanks. |
squakez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool work indeed. Thanks for the contribution!
|
✔️ Unit test coverage report - coverage increased from 50.6% to 50.7% (+0.1%) |
This PR implements automatic discovery of KEDA triggers (Kafka for now) from Camel integration source URIs.
Closes #6312
Changes
Behavior
When the KEDA trait is enabled, the system automatically extracts component information from
from()URIs and generates corresponding KEDA ScaledObject triggers without requiring manual trigger configuration.auto: true) when no manual triggers specifiedkeda.auto: falseto disable auto-discoverySupported Components
kafkakafkatopic,bootstrapServers,consumerGroupExample
With autoMetadata for extra KEDA params: