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

adapter: Fix views synchronizer bugs #1249

Open
wants to merge 1 commit into
base: Id1465b9c660965c898932c783f5e47064a50a4b7
Choose a base branch
from

Conversation

ethan-readyset
Copy link

@ethan-readyset ethan-readyset commented May 6, 2024

Previously, the views synchronizer only checked the server for views for
queries that were in the "pending" state. This meant that if the
migration handler set a query's state to "dry run succeeded" before the
views synchronizer had a chance to check the server for a view, the query
would be stuck in the "dry run succeeded" state forever, even if a view
for the query did indeed exist already.

This commit fixes the issue by having the views synchronizer check the
server for views for queries in either the "pending" or "dry run
succeeded" states. In order to prevent the views synchronizer from
rechecking every query with status "dry run succeeded" over and over
again, a "cache" has been added to the views synchronizer to keep track
of which queries have already been checked.

While working on this, I also noticed that it was possible for the
following sequence of events to occur:

  • Migration handler sees that a query is pending and kicks off a dry run
    migration
  • Views synchronizer finds a view on the server for the same query and
    sets the status to "successful"
  • Migration handler finishes the dry run migration for the query and
    overwrites the status as "dry run succeeded"

This could lead to a situation where a query that was previously
(correctly) labeled as "successful" is moved back to the "dry run
succeeded" state. To fix the issue, this commit updates the migration
handler to only write the "dry run succeeded" status if the query's
status is still "pending" after the dry run is completed.

Release-Note-Core: Fixed a bug where queries that already had caches
were sometimes stuck in the SHOW PROXIED QUERIES list

@ethan-readyset ethan-readyset changed the title adapter: Fix views synchronizer races adapter: Fix views synchronizer bug May 6, 2024
@readysetbot readysetbot force-pushed the Ie5faa100158fc80c906d8ad5cb897d8a02a07be9 branch from 2f1709d to e60957b Compare May 6, 2024 19:18
@ethan-readyset ethan-readyset changed the title adapter: Fix views synchronizer bug adapter: Fix views synchronizer bugs May 7, 2024
@readysetbot readysetbot force-pushed the Ie5faa100158fc80c906d8ad5cb897d8a02a07be9 branch 2 times, most recently from 4394921 to f9ceca3 Compare May 13, 2024 16:23
@ethan-readyset ethan-readyset changed the base branch from main to Id1465b9c660965c898932c783f5e47064a50a4b7 May 13, 2024 16:23
Previously, the views synchronizer only checked the server for views for
queries that were in the "pending" state. This meant that if the
migration handler set a query's state to "dry run succeeded" before the
views synchronizer had a chance to check the server for a view, the query
would be stuck in the "dry run succeeded" state forever, even if a view
for the query did indeed exist already.

This commit fixes the issue by having the views synchronizer check the
server for views for queries in *either* the "pending" or "dry run
succeeded" states. In order to prevent the views synchronizer from
rechecking every query with status "dry run succeeded" over and over
again, a "cache" has been added to the views synchronizer to keep track
of which queries have already been checked.

While working on this, I also noticed that it was possible for the
following sequence of events to occur:

- Migration handler sees that a query is pending and kicks off a dry run
  migration
- Views synchronizer finds a view on the server for the same query and
  sets the status to "successful"
- Migration handler finishes the dry run migration for the query and
  overwrites the status as "dry run succeeded"

This could lead to a situation where a query that was previously
(correctly) labeled as "successful" is moved back to the "dry run
succeeded" state. To fix the issue, this commit updates the migration
handler to only write the "dry run succeeded" status if the query's
status is still "pending" after the dry run is completed.

Release-Note-Core: Fixed a bug where queries that already had caches
  were sometimes stuck in the `SHOW PROXIED QUERIES` list
Change-Id: Ie5faa100158fc80c906d8ad5cb897d8a02a07be9
@readysetbot readysetbot force-pushed the Ie5faa100158fc80c906d8ad5cb897d8a02a07be9 branch from f9ceca3 to fb03811 Compare May 21, 2024 20:05
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