Skip to content

Conversation

DaniilAnichin
Copy link

Provide a description of what has been changed

Checklist

  • [*] When introducing a new scaler, I agree with the scaling governance policy
  • [*] I have verified that my change is according to the deprecations & breaking changes policy
  • [*] Tests have been added
  • [*] Changelog has been updated and is aligned with our changelog requirements
  • [*] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified) - I did my best to check, but it looks like no changes required
  • [*] A PR is opened to update the documentation on (repo) (if applicable)
  • [*] Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to keda-docs PR

@DaniilAnichin DaniilAnichin requested a review from a team as a code owner May 14, 2025 08:25
@DaniilAnichin DaniilAnichin force-pushed the feature/kafka-beginning-offset branch from 3791ea8 to d303e54 Compare May 14, 2025 08:39
@DaniilAnichin
Copy link
Author

DaniilAnichin commented May 14, 2025

Hi, I did my best to prepare the PR in the appropriate way (signed commits, added tests, added docs PR)
I'm not very good with go though, and I'm pretty sure my code changes itself are suboptimal.
I think it would be best to adjust the calls in such a way that earliest offsets are only loaded when the flag is set, but I did not get how exactly to do so, since it's done on the outside and passed as parameter, so we would need to type it as (some data or null) or something that direction.
I would love if someone close to the kafka scalers / project in general would advise on should I optimize it or it's good enough as is

Also I didn't touch experimental kafka-go scaler, to minimize back-n-force (assumed it would be better to polish one first, than copy to the second one if required)

P.S.: For what it's worth, I did build this version locally and deployed to the cluster, and it seems to work as expected so far

@DaniilAnichin
Copy link
Author

Based on other PRs, I'll tag @JorTurFer , @zroubalik and @dttung2905
Excuse me if this is exessive

@DaniilAnichin DaniilAnichin force-pushed the feature/kafka-beginning-offset branch from d303e54 to e879450 Compare May 14, 2025 09:33
@JorTurFer
Copy link
Member

The code looks correct, but I don't know anything about kafka, @dttung2905 or @zroubalik PTAL

Copy link
Contributor

@dttung2905 dttung2905 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So sorry for the late reply. I have put a 👀 icon on it and was actually reviewing the PR. But for some reason I left half way and totally forget about it 😞
LGTM too !

@DaniilAnichin
Copy link
Author

/run-e2e kafka

@dttung2905
Copy link
Contributor

dttung2905 commented Jun 1, 2025

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

Copy link

stale bot commented Aug 1, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Aug 1, 2025
@DaniilAnichin DaniilAnichin force-pushed the feature/kafka-beginning-offset branch from e879450 to 3270d87 Compare August 5, 2025 10:00
@DaniilAnichin
Copy link
Author

Hi, @dttung2905 , my late replies are stronger than your late replies; rebased, hopefully fixed the e2e tests, will try to update documentation PR today too.
Can I ask to schedule e2e kafka test again? Many thanks

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Aug 5, 2025
@dttung2905
Copy link
Contributor

Hi @DaniilAnichin

Hi, @dttung2905 , my late replies are stronger than your late replies

Not sure what you mean by this but let me help you with the e2e test as only KEDA members are allowed to run this

@dttung2905
Copy link
Contributor

dttung2905 commented Aug 5, 2025

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

@DaniilAnichin DaniilAnichin force-pushed the feature/kafka-beginning-offset branch from 3270d87 to ca8b808 Compare August 5, 2025 13:23
Signed-off-by: DaniilAnichin <[email protected]>
@DaniilAnichin DaniilAnichin force-pushed the feature/kafka-beginning-offset branch from ca8b808 to 9bcf53d Compare August 5, 2025 14:59
@DaniilAnichin
Copy link
Author

Hi @dttung2905 , that was a stupid joke about you saying "sorry for the late reply" earlier, and then me dissapearing for 2 month;

Unfortunately there were still problems in e2e tests, I hope current version of the branch will work as expected
Thanks for the quick response)

@DaniilAnichin
Copy link
Author

@dttung2905 Can you trigger /run-e2e kafka again? As you mentioned, I don't have privileges (reasonably) and such

@dttung2905
Copy link
Contributor

dttung2905 commented Aug 6, 2025

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

@DaniilAnichin
Copy link
Author

@dttung2905 @zroubalik I'm sorry for the inconventience, but I'm a bit stuck; I've updated the "obviously wrong" parts of the tests, and in my understanding it should be kinda working now; however, I wasn't able to find a local e2e testing guide, and I don't quite get how to possible debug a remove test setup.
To some extend I know that the changes made work as expected, because I have locally-build image running on dev cluster for past 2-3 month, but I would really love to have the tests that prove that & don't fail instead.

Maybe you can spot the reason why the testOneOnInvalidOffsetByMessageCount scales up even though the scaling metric should be 0 at the moment? Can it be that the deletion script doesn't work as expected, or maybe updates about deleted offsets are not propagated in time and the variant of time.sleep is required?

@dttung2905
Copy link
Contributor

Hi @DaniilAnichin ,

You actually can run the e2e test locally. It would be much faster and easier to debug . There is a guide on how to do so in https://github.com/kedacore/keda/blob/main/tests/README.md#running-tests

Also this is the e2e test file for kafka scaler for your reference. It sits in a different folder under tests https://github.com/kedacore/keda/tree/main/tests/scalers/kafka

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update here?

@DaniilAnichin
Copy link
Author

@zroubalik I temporarily ran out of time to work on the matter; Current status is: code changes seem to be ok-ish (works in custom build, etc.), however e2e tests fail; plan to debug locally, haven't manage to get there yet.

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.

4 participants