-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Kafka scaler: concurrent offset fetches #2405
Conversation
Signed-off-by: VerstraeteBert <[email protected]>
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.
Looking good, I wonder if we can cover this with some test.
@pierDipi @lionelvillard @matzew mind looking?
@bpinske PTAL |
It looks like the kafka e2e tests already include multiple partitions. So I think this should be covered by existing tests. https://github.com/kedacore/keda/blob/main/tests/scalers/kafka.test.ts#L242 |
Signed-off-by: VerstraeteBert <[email protected]>
Thanks for the feedback on my initial work by the way @bpinske Found out that there is a goroutine leak when an error is thrown, fixed it. Tests don't cover the failure scenario, however, I do think it's sound now. A second opinion would be great though. |
The logic looks correct to me. As long as the e2e tests continue to work, I think this is a great improvement. |
Signed-off-by: VerstraeteBert <[email protected]>
while i was working on #2409 , i had to setup the kafka e2e test locally, and it appears it runs a single node broker for the test. for this change, would it be beneficial to cover multi broker scenario? |
+1 |
Great to see a contribution from somebody in my neighborhood (also live near Bruges 😄 ) |
Signed-off-by: VerstraeteBert <[email protected]>
Signed-off-by: VerstraeteBert <[email protected]>
Signed-off-by: VerstraeteBert <[email protected]>
Thanks for peeking @pierDipi, @PaulLiang1 . Have changed the the Strimzi cluster CR to spawn 3 brokers. @tomkerkhove I see you're a howest alumni as well :-), cool! Could you trigger the e2e tests for me? |
Signed-off-by: VerstraeteBert <[email protected]>
/run-e2e kafka.test* |
Hi @VerstraeteBert , |
Co-authored-by: Jorge Turrado Ferrero <[email protected]> Signed-off-by: VerstraeteBert <[email protected]>
Signed-off-by: VerstraeteBert <[email protected]>
Might as well immediately concurrently fetch the consumer and producer offsets, while we're optimizing the metric generation. The I/O latency with the last commit changes from What do you guys think about this change? should I let |
I like it. The fewer round trips we need to wait on, the better.
Personally I think it would be best to keep the function structured as you have. It may make future other feature improvements easier if concurrency isn't too tightly coupled to the logic of collecting offsets. |
Sounds good, thanks for the feedback @bpinske . |
Signed-off-by: VerstraeteBert <[email protected]>
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.
LGTM, thanks a lot @VerstraeteBert
Signed-off-by: VerstraeteBert <[email protected]>
Signed-off-by: VerstraeteBert <[email protected]>
Signed-off-by: VerstraeteBert <[email protected]> Signed-off-by: alex60217101990 <[email protected]>
Signed-off-by: VerstraeteBert <[email protected]> Signed-off-by: alex60217101990 <[email protected]>
Concurrently query brokers for consumer and producer offsets
Checklist
Fixes #2377.
Signed-off-by: VerstraeteBert [email protected]