Skip to content

Conversation

@FGasper
Copy link
Collaborator

@FGasper FGasper commented Dec 10, 2024

The previous logic was backwards, which caused uint overflow.

This adds a test that usually passes without this change but should occasionally fail. (It should always pass, though, with this change.)

@FGasper FGasper requested a review from tdq45gj December 10, 2024 15:16
Copy link
Collaborator

@tdq45gj tdq45gj left a comment

Choose a reason for hiding this comment

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

The PR comment says it adds a test. Are we still adding it?

curTs, err := extractTimestampFromResumeToken(cs.ResumeToken())
if err == nil {
lagSecs := curTs.T - sess.OperationTime().T
lagSecs := int32(sess.OperationTime().T) - int32(curTs.T)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be prone to overflow. Can we convert T (uint32) to int64?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, good call

@FGasper FGasper requested a review from tdq45gj December 11, 2024 14:25
@FGasper
Copy link
Collaborator Author

FGasper commented Dec 11, 2024

@tdq45gj I added the test. (Somehow I omitted it inadvertently earlier.)

Copy link
Collaborator

@tdq45gj tdq45gj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@FGasper FGasper merged commit 3004e1d into mongodb-labs:main Dec 11, 2024
49 checks passed
@FGasper FGasper deleted the felipe_fix_lag_calculation branch December 11, 2024 15:02
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.

2 participants