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

Get avoid snapshot interval calculate int overflow #1088

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Jan 31, 2024

Motivation

When the user uses Integer.MAX_VALUE as keepNSnapshots, it will touch integer overflow, and persistence will never delete those events.

This change will not change anything besides avoid overflow, because the persistence actor version already designed as a long type.

This screenshot was happened in the real world:

image

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Lgtm

@He-Pin He-Pin added the bug Something isn't working label Jan 31, 2024
@He-Pin He-Pin added this to the 1.1.0-M1 milestone Jan 31, 2024
Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

Lgtm. I think this is a bug fix and should ideally be backported to 1.0.x branch too.

@pjfanning
Copy link
Contributor

@Roiocam Probably hard to test but could you consider if it's possible?

@He-Pin
Copy link
Member

He-Pin commented Jan 31, 2024

@Roiocam just saw this in production @pjfanning

@He-Pin He-Pin merged commit 7dd5d73 into apache:main Jan 31, 2024
18 checks passed
@He-Pin
Copy link
Member

He-Pin commented Jan 31, 2024

@Roiocam Would you like to cherry pick this to 1.0.x ? Thanks.

@pjfanning pjfanning modified the milestones: 1.1.0-M1, 1.0.x Jan 31, 2024
@Roiocam Roiocam deleted the avoid-snapshot-interval-overflow branch February 1, 2024 02:00
@Roiocam
Copy link
Member Author

Roiocam commented Feb 1, 2024

@Roiocam Probably hard to test but could you consider if it's possible?

Yes, the reproduction can be implemented on persistence-plugin, I will pick it up and check it out if any possible to implement on pekko-persistence

Roiocam added a commit to Roiocam/pekko that referenced this pull request Feb 1, 2024
@He-Pin
Copy link
Member

He-Pin commented Feb 1, 2024

There is no need to write a plugin for just test this...

@Roiocam
Copy link
Member Author

Roiocam commented Feb 1, 2024

There is no need to write a plugin for just test this...

I mean verified in such as pekko-persistence-jdbc

He-Pin pushed a commit that referenced this pull request Feb 1, 2024
@pjfanning pjfanning added the release notes Need to release note label Feb 5, 2024
@He-Pin He-Pin modified the milestones: 1.0.x, 1.1.0-M1 Mar 18, 2024
@pjfanning pjfanning removed the release notes Need to release note label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants