-
Notifications
You must be signed in to change notification settings - Fork 204
Store tester stability #2060
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
base: main
Are you sure you want to change the base?
Store tester stability #2060
Conversation
| let client = self.client_pool.next(); | ||
| let config = client.client_config(); | ||
| if config.mocks.is_none() { | ||
| if config.mocks.is_none() && !client.is_connected() { |
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 means Fred isn't logging every time we call this
711f91a to
f2c36b6
Compare
MarcusSorealheis
left a comment
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.
Provided you can explain why we have commented out the CI workflow, that's fine. We have never used FIXME in this repo, or not that I can recall. Otherwise, I'm good with it.
| fi | ||
| shell: bash | ||
|
|
||
| # FIXME(palfrey): Can't make this reliably run in CI |
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.
why?
| let res = store_clone.get_and_decode(data.clone()).await?; | ||
| if let Some(existing_data) = res { | ||
| data.version = existing_data.version + 1; | ||
| data.version = existing_data.version; |
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.
Didn't realize we tried to hack optimistic locking here. This change should prevent race conditions if the version was already up to date. Have you verified that this does not break the update logic of your tester?
MarcusSorealheis
left a comment
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.
There is some issue in the queueing mechanism.
Description
Please include a summary of the changes and the related issue. Please also
include relevant motivation and context.
Fixes # (issue)
Type of change
Please delete options that aren't relevant.
not work as expected)
How Has This Been Tested?
Please also list any relevant details for your test configuration
Checklist
bazel test //...passes locallygit amendsee some docsThis change is