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

[scd] inline cleanup of implicit subscription into CRDB call when removing OIR #1060

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Jul 29, 2024

This is the first part of #1059:

It quite literally moves the Go logic into SQL, by means of nested queries and conditional DELETE statements.

(The PR sits on top of #1057)

Test plan:

this PR survives a run of every current uss_qualifier configuration when run locally and also passes the prober (both on CI and locally)

@Shastick Shastick force-pushed the inline-oir-deletion-implicit-removal branch 2 times, most recently from daf4e31 to 49170e3 Compare August 5, 2024 11:01
@Shastick Shastick marked this pull request as ready for review August 5, 2024 11:10
@Shastick Shastick force-pushed the inline-oir-deletion-implicit-removal branch 3 times, most recently from 55a3ca5 to ad28b74 Compare August 6, 2024 07:11
@mickmis
Copy link
Contributor

mickmis commented Aug 22, 2024

I wanted to start the review of this PR but:

  1. Is it appropriate to start relying more on CRDB given the current pending decisions? cc @BenjaminPelletier
  2. Is it reviewable in current state? It looks out of date from [scd] properly cleanup implicit sub on oir update #1057 @Shastick

@BenjaminPelletier
Copy link
Member

I do think we want to continue to maintain our current products until we have a positive new direction. We probably don't want to focus on optimizations targeted specifically for CRDB (though it's probably still valuable to complete outstanding work even if it did target CRDB specifically), but it seems like this improvement would be mostly applicable for most alternative SQL options.

@Shastick
Copy link
Contributor Author

Is it reviewable in current state? It looks out of date from #1057 @Shastick

This PR does not strictly need to be up to date with #1057 as it is going to replace the Go logic from #1057 with subqueries: I would definitely wait for the other to be merged before merging this one, but the approach using subqueries can already be discussed and reviewed.

@Shastick Shastick marked this pull request as draft September 3, 2024 10:43
@Shastick Shastick force-pushed the inline-oir-deletion-implicit-removal branch from ad28b74 to ee8be76 Compare September 9, 2024 11:49
@Shastick
Copy link
Contributor Author

Shastick commented Sep 9, 2024

Replaced with #1107

@Shastick Shastick closed this Sep 9, 2024
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.

3 participants