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

Fixes #158 #159

Merged
merged 3 commits into from
Aug 6, 2022
Merged

Fixes #158 #159

merged 3 commits into from
Aug 6, 2022

Conversation

Gsantomaggio
Copy link
Member

@Gsantomaggio Gsantomaggio commented Aug 4, 2022

Signed-off-by: Gabriele Santomaggio [email protected]

The PR fixes the wrong offset stored.
It is in draft since tests are missing.

@Gsantomaggio Gsantomaggio requested a review from Zerpet August 4, 2022 16:15
@Gsantomaggio Gsantomaggio marked this pull request as draft August 4, 2022 16:15
@codecov
Copy link

codecov bot commented Aug 4, 2022

Codecov Report

Merging #159 (a691cff) into main (1ac7034) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #159   +/-   ##
=======================================
  Coverage   80.07%   80.07%           
=======================================
  Files          17       17           
  Lines        2575     2570    -5     
=======================================
- Hits         2062     2058    -4     
+ Misses        371      370    -1     
  Partials      142      142           
Impacted Files Coverage Δ
pkg/stream/consumer.go 80.20% <ø> (+0.10%) ⬆️
pkg/stream/coordinator.go 93.78% <ø> (ø)
pkg/stream/client.go 83.69% <100.00%> (ø)
pkg/stream/server_frame.go 81.98% <100.00%> (-0.06%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Zerpet Zerpet changed the title Fixes https://github.com/rabbitmq/rabbitmq-stream-go-client/issues/158 Fixes #158 Aug 5, 2022
@Zerpet Zerpet linked an issue Aug 5, 2022 that may be closed by this pull request
Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

This looks good, and I think it's non-breaking since the signature of MessageHandler() doesn't change. I'm going to add a test case to ensure the correctness of the fix.

Gsantomaggio and others added 2 commits August 5, 2022 13:02
Signed-off-by: Gabriele Santomaggio <[email protected]>
Signed-off-by: Aitor Perez Cedres <[email protected]>
@Zerpet Zerpet force-pushed the fix_wrong_offset branch from 9dfea91 to f492854 Compare August 5, 2022 12:10
@Zerpet
Copy link
Contributor

Zerpet commented Aug 5, 2022

Rebased main branch to resolve conflicts. I will do a refactor before marking this ready.

Related to #158

Signed-off-by: Aitor Perez Cedres <[email protected]>
@Zerpet Zerpet marked this pull request as ready for review August 5, 2022 13:10
@Zerpet
Copy link
Contributor

Zerpet commented Aug 5, 2022

Marking as ready for review after adding a test case for the fix. @Gsantomaggio do you want to review the test case once CI passes?

@Zerpet Zerpet added this to the 1.0.1 milestone Aug 5, 2022
@Zerpet Zerpet added the v1.x Issues specific for version 1.x label Aug 5, 2022
@Gsantomaggio
Copy link
Member Author

@Zerpet looks good to me! ty!

@Gsantomaggio Gsantomaggio merged commit 345b205 into main Aug 6, 2022
@Gsantomaggio Gsantomaggio deleted the fix_wrong_offset branch August 6, 2022 15:31
@kensou97
Copy link
Contributor

kensou97 commented Aug 7, 2022

Will it be released recently?

@Gsantomaggio
Copy link
Member Author

@kensou97
Copy link
Contributor

kensou97 commented Aug 7, 2022

Thank you, i'll have a try

@kensou97
Copy link
Contributor

kensou97 commented Aug 8, 2022

Can you try this code? The message with offset 100 is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.x Issues specific for version 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong offset reported
3 participants