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.

Sign up for GitHub

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

Jump to bottom

fix: set local schema when syncing from remote #3956

Merged
merged 1 commit into from
Oct 10, 2023
Merged

fix: set local schema when syncing from remote #3956

merged 1 commit into from
Oct 10, 2023

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Oct 9, 2023 •
edited
Loading

Description

  • When syncing from remote, we need to set the local schema as well.

Linear Ticket

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@achettyiitr achettyiitr marked this pull request as draft October 9, 2023 17:29
@achettyiitr achettyiitr marked this pull request as ready for review October 10, 2023 06:14
@achettyiitr achettyiitr mentioned this pull request Oct 10, 2023
Closed
1 task
@codecov
Copy link

codecov bot commented Oct 10, 2023 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (08d036e) 70.72% compared to head (2285592) 70.80%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3956      +/-   ##
==========================================
+ Coverage   70.72%   70.80%   +0.08%     
==========================================
  Files         357      357              
  Lines       53671    53676       +5     
==========================================
+ Hits        37960    38007      +47     
+ Misses      13449    13416      -33     
+ Partials     2262     2253       -9
Files Coverage Δ
warehouse/schema/schema.go 98.97% <98.03%> (+4.16%) ⬆️

... and 12 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fracasula fracasula changed the base branch from master to release/1.15.x October 10, 2023 06:33
@fracasula fracasula changed the base branch from release/1.15.x to master October 10, 2023 06:35
@fracasula
Copy link
Collaborator

@achettyiitr can you redo this on release/1.15.x?

@fracasula
Copy link
Collaborator

It's hard to tell what you changed because you renamed the methods receivers. You did that all in one commit so can you either not rename the method receiver or do it in a separate commit please?

lvrach
lvrach approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

This refactoring and the addition of tests is the right step forward.

In my review, I checked for the locks we are currently using, but I am not certain about all of them.

We should invest in simplifying this code further, and understand if those many locks are indeed needed.

warehouse/schema/schema.go Outdated
Comment on lines 262 to 264
s.localSchemaMu.Lock()
s.localSchema = updatedSchema
s.localSchemaMu.Unlock()
Copy link
Member

Choose a reason for hiding this comment

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

What is the need for maintaining s.localSchema?
Is it for performance reasons, to avoid database queries?
Would it make sense to use the database, and figure out caching in different layers or avoid caching all together?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can simplify the logic by separating the caching into a different layer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Database queries can cost us because in some cases even the queries to INFORMATION_SCHEMAS tables take a long time.

Copy link
Member Author

@achettyiitr achettyiitr Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Added a task in the Backlog so that we can revisit this again.

@achettyiitr
Copy link
Member Author

achettyiitr commented Oct 10, 2023 •
edited
Loading

It's hard to tell what you changed because you renamed the methods receivers. You did that all in one commit so can you either not rename the method receiver or do it in a separate commit, please?

Sure. Also, I added an existing test in the master to verify. #3957 and it's failing as expected.

fracasula
fracasula approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@fracasula fracasula left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @achettyiitr 👍

We're targeting master here though but yesterday I had created a release and a prerelease.

If you guys (cc @lvrach) are planning to merge this into master please close them. We need to get some closure on this though, it's starting to become confusing.

@achettyiitr achettyiitr merged commit 6dd93d4 into master Oct 10, 2023
35 checks passed
@achettyiitr achettyiitr deleted the fix.sync-remote-schema branch October 10, 2023 07:51
This was referenced Oct 11, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lvrach lvrach lvrach approved these changes

@fracasula fracasula fracasula approved these changes

Assignees
No one assigned
Labels
warehouse-team with tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@achettyiitr @fracasula @lvrach