Skip to content
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

fix(azure event hub): wrong calc of the remaining items #3912

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

yodobrin
Copy link
Contributor

@yodobrin yodobrin commented Nov 24, 2022

as the scenario covered in this line is for the case in which the partition reached maxInt and started from zero. the calc is done wrongly.
image

Signed-off-by: Yoav Dobrin 37622785+yodobrin@users.noreply.github.com

the calc of the remaining items to process is wrong

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

as the scenario covered in this line is for the case in which the partition reached maxInt and started from zero. the calc is done wrongly.

Signed-off-by: Yoav Dobrin <37622785+yodobrin@users.noreply.github.com>
@yodobrin yodobrin requested a review from a team as a code owner November 24, 2022 13:13
@JorTurFer
Copy link
Member

JorTurFer commented Nov 24, 2022

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

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! ❤️
LGTM, but I'm not an expert in EH, @v-shenoy , do you know if this change is correct?

@yodobrin
Copy link
Contributor Author

The most common scenario would be that the partition.lastseq would be larger than the checkpoint.seq – this is addressed in previous lines of code.
Since the partition is a cyclic counter, once it reaches maxInt it will zero, in that scenario the checkpoint.seq will be larger.
Since we try to calc the number of records we need to process

Lets say max is 100
Partition.lastseq is 3
Checkpoint.seq is 6

How many records do we need to process?
In the current calc we have 100-3+6=103
In the suggested we have 100-6+3=97

Since the number of records from 6 to 100 is 94, and the number of newly created records is 3, the number of records we need to handle is 97, not 103.

@JorTurFer
Copy link
Member

JorTurFer commented Nov 24, 2022

The most common scenario would be that the partition.lastseq would be larger than the checkpoint.seq – this is addressed in previous lines of code. Since the partition is a cyclic counter, once it reaches maxInt it will zero, in that scenario the checkpoint.seq will be larger. Since we try to calc the number of records we need to process

Lets say max is 100 Partition.lastseq is 3 Checkpoint.seq is 6

How many records do we need to process? In the current calc we have 100-3+6=103 In the suggested we have 100-6+3=97

Since the number of records from 6 to 100 is 94, and the number of newly created records is 3, the number of records we need to handle is 97, not 103.

I have been checking your numbers and makes sense in my mind (I'm not really good at mathematics imaging shapes TBH), let's way till other folk confirms this and we can merge it.
Thanks for the sample you have provided, it's really useful

@v-shenoy
Copy link
Contributor

Thanks for the contribution! ❤️
LGTM, but I'm not an expert in EH, @v-shenoy , do you know if this change is correct?

I will spend some time on this later today, and get back.

@yodobrin
Copy link
Contributor Author

yodobrin commented Dec 1, 2022

Hi @v-shenoy and @JorTurFer - please update with your decsion

@JorTurFer
Copy link
Member

JorTurFer commented Dec 2, 2022

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

@JorTurFer
Copy link
Member

JorTurFer commented Dec 2, 2022

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

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM!
Let's wait some time for giving the option to @v-shenoy to check this if he has time, and if not I'll merge it in a few days

@v-shenoy
Copy link
Contributor

v-shenoy commented Dec 5, 2022

LGTM! Let's wait some time for gibing the option to @v-shenoy to check this if he has time, and if not I'll merge it in a few days

I am not completely sure about this either, but this seems right.

@JorTurFer JorTurFer changed the title wrong calc of the remaining items fix(azure event hub): wrong calc of the remaining items Dec 5, 2022
@JorTurFer JorTurFer merged commit 6e17fae into kedacore:main Dec 5, 2022
josephangbc pushed a commit to josephangbc/keda that referenced this pull request Dec 6, 2022
as the scenario covered in this line is for the case in which the partition reached maxInt and started from zero. the calc is done wrongly.

Signed-off-by: Yoav Dobrin <37622785+yodobrin@users.noreply.github.com>

Signed-off-by: Yoav Dobrin <37622785+yodobrin@users.noreply.github.com>
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.

3 participants