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

Multiple db support for REST scenario #2169

Merged
merged 16 commits into from May 8, 2024
Merged

Multiple db support for REST scenario #2169

merged 16 commits into from May 8, 2024

Conversation

rohkhann
Copy link
Contributor

@rohkhann rohkhann commented Apr 16, 2024 •
edited

Why make this change?

This change introduces REST support for multiple DB Scenario.
Issue: #1753

What is this change?

  1. Instead of using the default db name to dispatch queries, we are using the db name based on entity and then dispatching queries and mutations.

How was this tested?

  1. Existing unit tests should cover backward compatibility scenarios of all single db rest support.
  2. Existing unit tests cover the flow once the query is dispatched to the queryEngine/query executor. We need to add the tests for the integration scenario for other calls in rest.
  3. Added test for rest scenario dispatch routing.

Integration test:

  1. Two db's one has publishers entity and one has books:
    Get publishers
    image

Get books
image

@rohkhann
Copy link
Contributor Author

/azp run

@seantleonard seantleonard added this to the 0.12rc2 milestone Apr 16, 2024
@rohkhann
Copy link
Contributor Author

/azp run

@JerryNixon
Copy link
Contributor

No doubt, every entity referenced in a query could have a different database/source. Great fix.

seantleonard
seantleonard reviewed Apr 25, 2024
View reviewed changes
Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

few questions, otherwise looks good. Have you tested this in end2end scenario outside of unit test? I understand that testing that isn't doable/straighforward in pipelines for cross-db types (e.g. sql -> cosmos) because we only have one db type per pipeline. But for multiple sql sources, that may be possible? e.g. different db's on same server and each db is represented as two distinct datasources.

src/Core/Services/RestService.cs Show resolved Hide resolved
src/Service.Tests/Unittests/MultiSourceQueryExecutionUnitTests.cs Outdated Show resolved Hide resolved
@seantleonard seantleonard modified the milestones: 0.12rc2, 1.1rc Apr 30, 2024
@rohkhann
Copy link
Contributor Author

rohkhann commented May 1, 2024

/azp run

@rohkhann
Copy link
Contributor Author

rohkhann commented May 1, 2024

/azp run

@rohkhann
Copy link
Contributor Author

rohkhann commented May 1, 2024

/azp run

@rohkhann
Copy link
Contributor Author

rohkhann commented May 1, 2024

/azp run

aaronburtle
aaronburtle reviewed May 7, 2024
View reviewed changes
Copy link
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

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

Looks good so far, still need to finish going through the tests.

ayush3797
ayush3797 approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

LGTM after addressing Aaron's comments!

@rohkhann
Copy link
Contributor Author

rohkhann commented May 7, 2024

/azp run

@seantleonard seantleonard linked an issue May 7, 2024 that may be closed by this pull request
[Improvement] Multi-Source Support for REST scenario. #1753
Closed
aaronburtle
aaronburtle approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@aaronburtle aaronburtle left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing things quickly!

@JerryNixon
Copy link
Contributor

Some smart people on this PR thread

seantleonard
seantleonard reviewed May 8, 2024
View reviewed changes
Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

additional questions about tests.

src/Service.Tests/Unittests/MultiSourceQueryExecutionUnitTests.cs Outdated Show resolved Hide resolved
src/Service.Tests/Unittests/MultiSourceQueryExecutionUnitTests.cs Show resolved Hide resolved
src/Service.Tests/Unittests/MultiSourceQueryExecutionUnitTests.cs Outdated Show resolved Hide resolved
@rohkhann
Copy link
Contributor Author

rohkhann commented May 8, 2024

/azp run

@seantleonard seantleonard enabled auto-merge (squash) May 8, 2024 17:55
@seantleonard seantleonard merged commit 910ba2a into main May 8, 2024
7 checks passed
@seantleonard seantleonard deleted the rohkhann/RestsupportMultipleDbs branch May 8, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seantleonard seantleonard seantleonard approved these changes

@ayush3797 ayush3797 ayush3797 approved these changes

@aaronburtle aaronburtle aaronburtle approved these changes

@mbhaskar mbhaskar Awaiting requested review from mbhaskar mbhaskar is a code owner

@Aniruddh25 Aniruddh25 Awaiting requested review from Aniruddh25 Aniruddh25 is a code owner

@Mathos1432 Mathos1432 Awaiting requested review from Mathos1432 Mathos1432 is a code owner

@gledis69 gledis69 Awaiting requested review from gledis69 gledis69 is a code owner

@tarazou9 tarazou9 Awaiting requested review from tarazou9 tarazou9 is a code owner

@abhishekkumams abhishekkumams Awaiting requested review from abhishekkumams abhishekkumams is a code owner

@aaronpowell aaronpowell Awaiting requested review from aaronpowell aaronpowell is a code owner

@severussundar severussundar Awaiting requested review from severussundar severussundar is a code owner

@ravishetye ravishetye Awaiting requested review from ravishetye ravishetye is a code owner

@neeraj-sharma2592 neeraj-sharma2592 Awaiting requested review from neeraj-sharma2592 neeraj-sharma2592 is a code owner

@sourabh1007 sourabh1007 Awaiting requested review from sourabh1007 sourabh1007 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1rc
Development

Successfully merging this pull request may close these issues.

[Improvement] Multi-Source Support for REST scenario.
6 participants
@rohkhann @JerryNixon @seantleonard @ayush3797 @aaronburtle @Aniruddh25