-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ws-manager-bridge] Fix statusVersion comparison #9233
Conversation
return; | ||
} | ||
// prebuild.statusVersion = 0 is the default value in the DB, these shouldn't be counted as stale in our metrics | ||
if (prebuild.statusVersion > 0 && prebuild.statusVersion >= status.statusVersion) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change. We skip any records which have prebuild.statusVersion == 0
(this comes from the db) because 0 is the default DB value. And we then check that prebuild status is greater than our new event, so that we bail early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, just to make sure I understand this properly, status.statusVersion
can never be 0
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that's correct.
Let's assume that it can be 0, then:
The only way we get to a comparison of prebuild db record
and event
is if prebuild.statusVersion > 0
.
If this is true, than prebuild.statusVersion
must be at least 1, or greater. In this case, that means that we must have already processed an event once to change it from 0, to 1+ (0 is the DB default value, set when the Prebuild record is initially created by the server). In this case, that means that we're good even if status.statusVersion
can be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic change looks good to me, and trusting your test run. Many thanks for the quick fix!
Description
The comparison logic was the wrong way. We'd check the statusVersion stored in the DB <= new update and exit early if it was. That's the incorrect behaviour as status.statusVersion is monotonically increasing
Related Issue(s)
Fixes #
How to test
Release Notes
Documentation
NONE