Skip to content

Conversation

@sadath-12
Copy link
Contributor

@sadath-12 sadath-12 commented Aug 14, 2024

Summary

closes #1143

Test Plan

  • PR has an associated issue: #
  • make check passes
  • make test shows 100% unit test coverage

Deployment Plan

@netlify
Copy link

netlify bot commented Aug 14, 2024

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
🔨 Latest commit 89980f6
🔍 Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/66bc78deab37060008a5bd9f

Signed-off-by: sadath-12 <[email protected]>
Signed-off-by: sadath-12 <[email protected]>
Copy link
Contributor

@shangyian shangyian left a comment

Choose a reason for hiding this comment

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

This does make sense, but I can see that some assumptions made by the query service will be different. One original assumption was that it would call the core API to post changes rather than touching the metadata db, but it can now just make changes in the metadata database directly.

Also this change will need to handle multi-tenancy as well, related to #1122, since there would need to be a separate query service attached to each tenant of the core service.

Copy link
Contributor

@shangyian shangyian left a comment

Choose a reason for hiding this comment

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

I think you'll want to run make check (linters) and make test. Some of the tests are failing due to the naming change from Catalog -> QSCatalog

@samredai
Copy link
Contributor

Thanks @sadath-12! I'll review this soon and also pull it locally to try it out.

One original assumption was that it would call the core API to post changes rather than touching the metadata db, but it can now just make changes in the metadata database directly.

I'm hoping we can stick with this assumption and avoid having one of the services directly modify metadata for another service. It's nice that this enables sharing a single postgres db for the metadata but it's still worth allowing them to be separate postgres instances if someone wants to use it that way.

@samredai
Copy link
Contributor

@sadath-12 take a look at #1168 when you have a chance. This removes the requirement to do this rename and greatly simplifies the query service. Now it only relies on a single query table (which does not conflict with the main service). Now, based on how you want to deploy it, it can share a database instance with the core service or run on an entirely separate one.

I know this PR was meant to unblock #1140 which is also achieved by the new PR. Let's revisit that one after this refactor is merged. It should be much easier to make the changes necessary to get a good multi-tenancy setup working for you.

@sadath-12 sadath-12 closed this Oct 2, 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.

Rename Query Service metadata tables to not conflict with core service table names

3 participants