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

Restoring previous version of pruning (without left join) #1544

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

mateusz-sekara
Copy link
Contributor

@mateusz-sekara mateusz-sekara commented Nov 20, 2024

Motivation

Outer joins are putting too much pressure on the database. There were some fixes done to improve that, but in 2.18 release. In the meantime, I'm bringing back the previous implementation until we merge 2.18 to CCIP repo. The only difference is that, we not gonna drop the orphaned logs (it would work as the currently released version)

Using outer join

image

Restoring inner join

image

@mateusz-sekara mateusz-sekara requested review from a team as code owners November 20, 2024 17:11
@mateusz-sekara mateusz-sekara requested review from winder, rstout, asoliman92, makramkd, dimkouv, 0xAustinWang, 0xnogo and reductionista and removed request for a team November 20, 2024 17:11
Comment on lines -509 to -512
// It should have retained the log matching filter0 (due to ret=0 meaning permanent retention) as well as all
// 3 logs matching filter12 (ret=1 hour). It should have deleted 3 logs not matching any filter, as well as 1
// of the 2 logs matching filter1 (ret=1ms)--the one that doesn't also match filter12.
assert.Len(t, logs, 4)
Copy link
Collaborator

@reductionista reductionista Nov 20, 2024

Choose a reason for hiding this comment

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

I think line 512 and the comment above it are the only lines that actually need to be reverted here. The rest of the changes to this file are all useful improvements to the test that would be relevant with either version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Marking as approved since this doesn't matter much either way--I'll leave it up to you, if it's easier just to revert the whole thing for now and then re-apply the same commit later that's fine

@mateusz-sekara mateusz-sekara merged commit a1636a9 into release/2.17.0-ccip1.5 Nov 21, 2024
156 of 157 checks passed
@mateusz-sekara mateusz-sekara deleted the ms/bring-back-inner-join branch November 21, 2024 07:41
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