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

[ScalarDB .NET Client] Add microservice transactions sample with Shared-cluster pattern with LINQ #72

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Dima-X
Copy link
Collaborator

@Dima-X Dima-X commented Sep 30, 2024

Description

This PR moves a microservice transactions sample with the shared-cluster pattern with LINQ from scalardb-cluster-dotnet-client-sdk
.
This is a .NET version of #64.

The sample made in a similar manner to it’s Java counterpart, but it has 2 additional projects:

  • Common: contains C# classes which represents database schema, and some common exceptions
  • DataLoader: app to initialize database schema, create users with needed privileges and load initial data

Related issues and/or PRs

https://github.com/scalar-labs/docs-internal-scalardb/pull/456
https://github.com/scalar-labs/scalardb-cluster-dotnet-client-sdk/pull/110#issuecomment-2240906449

Changes made

  • sample added to dotnet/microservice-transactions-sample-with-shared-cluster-with-linq
  • Visual Studio specific exceptions were added to .gitignore

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

The main difference with scalardb-cluster-dotnet-client-sdk:

  • projects reference 3.13 version of ScalarDB.Client from NuGet
  • docker-compose.yml: scalardb-cluster-node-byol-premium:3.13.0 is referenced

@Dima-X Dima-X self-assigned this Sep 30, 2024
@Dima-X Dima-X requested review from a team, komamitsu, brfrn169, feeblefakie, Torch3333, kota2and3kan and josh-wong and removed request for a team September 30, 2024 06:54
Copy link
Member

@josh-wong josh-wong left a comment

Choose a reason for hiding this comment

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

I've left some minor suggestions regarding wording in some places, but I wonder if they're not specific to this PR (meaning that those suggestions might apply to the other samples).

Other than that, LGTM. Thank you!🙇🏻‍♂️

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I compared this and https://github.com/scalar-labs/scalardb-cluster-dotnet-client-sdk/pull/110 that I've already approved. There is no significant diff between them.

BTW, this PR adds a top-level directory dotnet to group sample applications by language. But, grouping by the type of sample applications (e.g., multi-storage) could be another option. We might need some discussion on it.

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Sorry, I submitted the approval before review. I will take a look.

@feeblefakie feeblefakie self-requested a review October 1, 2024 12:55
Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Contributor

@kota2and3kan kota2and3kan left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

FYI: I was able to run this sample application in my local environment based on the new document.

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

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.

7 participants