Refactor get config to getParameterFromConfigV2 Kafka Scaler#5319
Refactor get config to getParameterFromConfigV2 Kafka Scaler#5319dttung2905 wants to merge 11 commits intokedacore:mainfrom
getParameterFromConfigV2 Kafka Scaler#5319Conversation
|
Thank you for your contribution! 🙏 We will review your PR as soon as possible. While you are waiting, make sure to:
Learn more about: |
getParameterFromConfigV2 Kafka ScalergetParameterFromConfigV2 Kafka Scaler
fa091b2 to
8b8a4f2
Compare
getParameterFromConfigV2 Kafka ScalergetParameterFromConfigV2 Kafka Scaler
wozniakjan
left a comment
There was a problem hiding this comment.
can you please add a changelog entry? I guess it should go in the other section as Kafka scaler: ...
8b8a4f2 to
f6bd0fd
Compare
| tlsString, err := getParameterFromConfigV2( | ||
| config, | ||
| "tls", | ||
| reflect.TypeOf(""), | ||
| UseMetadata(true), | ||
| UseAuthentication(true), | ||
| UseResolvedEnv(false), | ||
| IsOptional(true), | ||
| WithDefaultVal("disable"), |
There was a problem hiding this comment.
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?
| 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"), |
There was a problem hiding this comment.
I think eleminating option function when it is not true make sense as we already initiate the default value in this
Line 243 in ef46459
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
|
I would personally just drop the argument from all would become the bool argument is imho not necessary and the absence of |
f6bd0fd to
ae7776b
Compare
|
@JorTurFer Could you help to take a look at this when you have time ? |
|
Sorry, I've been travelling this week :( I agree with the suggestions from @wozniakjan , but you've already addressed them, so nothing else from my side 🥇 |
|
/run-e2e kafka |
|
let me take a look at the failed e2e tests |
|
/run-e2e kafka |
|
Seems like the similar failures is happening on the daily E2E test on The current failed E2E test shows similar pattern ( not exactly the same test case but still within the apache kafka scaler test ) The errors come from this line @JorTurFer Do you know how can we can increase the timeout to greater than |
Actually, the timeout is 20m: Lines 27 to 33 in f2d86a8 |
|
let me rebase it . Hopefully the flaky test is gone from this PR #5610 |
2bd0ae2 to
6158216
Compare
6158216 to
466f0ed
Compare
|
/run-e2e |
|
/run-e2e kafka |
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
Signed-off-by: dttung2905 <ttdao.2015@accountancy.smu.edu.sg>
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>
466f0ed to
218c15f
Compare
|
/run-e2e kafka |
zroubalik
left a comment
There was a problem hiding this comment.
@wozniakjan so are we gonna give this a try?
|
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 |
|
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. |
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 |
|
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. |
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