Skip to content

Conversation

@Dima-X
Copy link
Contributor

@Dima-X Dima-X commented Mar 28, 2025

Description

This PR adds sample application for ScalarDB Cluster .NET Client SDK.
This is a .NET version of https://github.com/scalar-labs/scalardb-samples/tree/main/scalardb-sample

Related issues and/or PRs

https://github.com/scalar-labs/docs-internal-scalardb/pull/1051

Changes made

  • sample added to dotnet/scalardb-cluster-sample
  • Visual Studio specific exceptions were added to .gitignore

Checklist

  • 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)

@Dima-X Dima-X self-assigned this Mar 28, 2025
@Dima-X Dima-X requested review from a team, Torch3333, brfrn169, feeblefakie and komamitsu and removed request for a team March 28, 2025 08:03
loadInitialDataCommand.SetHandler(async () =>
{
using var sample = new Sample();
await sample.CreateTables();
Copy link
Contributor

Choose a reason for hiding this comment

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

The existing sample applications don't create tables within the applications, as table creation is handled by the Schema Loader tool. I think this sample application should follow the same approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think forcing .NET developers to install Java just to use the Schema Loader tool is not good; that’s why tables are created within the sample application.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have a point. Maybe it's a trade off between the consistency with other sample applications and the convenience of this one. I don't have a strong opinion on it, so I'll leave the decision to @feeblefakie and @brfrn169.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dima-X Can you make it configurable?
So, users can create tables through this (default) or create tables through the schema-loader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feeblefakie Tables already can be created both ways. Tables and namespaces are created only if they don’t exist already, so there’s no problem applying the schema with the schema-loader and then using this sample app.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, table schema migration is basically conducted outside applications in production environments. So I think the tables for the sample application should be explicitly created as much as possible. On the other hand, I understand that it's might be not easy for .NET developers to use the Schema Loader or equivalent. How about adding a new command CreateTablesCommand instead of implicitly creating the tables in LoadInitialDataCommand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@komamitsu

How about adding a new command CreateTablesCommand instead of implicitly creating the tables in LoadInitialDataCommand?

I thought about doing it as a separate command at first, but you always have to run LoadInitialDataCommand exactly after CreateTablesCommand, so I think it’s more logical for them to be a single command.

Copy link
Contributor

Choose a reason for hiding this comment

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

it’s more logical for them to be a single command.

This depends on whether Scalar wants to provide a sample application that always implicitly creates the tables. I'll wait for feedback from other members.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion with @feeblefakie and @brfrn169, we decided to use a Docker image that runs the Schema Loader tool after we make it public. So, let's go with the current PR as is.

);

if (customer is not null)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the target record exist and credit_total has been modified from the initial value, I think it should be reset to the initial value after LoadInitialData finishes. How about updating or deleting and re-inserting it if it exists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think that the existing Java sample application is doing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. That's fair enough.

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!

@Dima-X Dima-X requested a review from komamitsu March 31, 2025 07:37
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! 👍

@brfrn169 brfrn169 requested a review from kota2and3kan April 2, 2025 01:47
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!

@@ -0,0 +1,31 @@
using System.CommandLine;
Copy link
Contributor

Choose a reason for hiding this comment

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

As for the location, let's put them into scalardb-dotnet-samples instead of scalardb-samples/dotnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feeblefakie
Created public scalardb-dotnet-samples repository.
Will move this sample there after there are no more questions and requests about the sample. Or is it better to move it right now and set all of you as reviewers again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I'm sorry, I made a mistake.
I meant the scalardb-dotnet-samples folder in the scalardb-samples repo.
My bad.

Copy link
Contributor Author

@Dima-X Dima-X Apr 3, 2025

Choose a reason for hiding this comment

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

@feeblefakie
Moved to scalardb-dotnet-samples

@komamitsu
Copy link
Contributor

komamitsu commented Apr 4, 2025

@feeblefakie @brfrn169 @Torch3333 I added a small fix to throw the last exception when the retry is over 18e99de and b0438af. PTAL just in case.

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.

Thank you for creating the new sample application!
It's a minor thing, but I have questions about an error that I faced in my local environment.
Please take a look when you have time! 🙇‍♂️

Comment on lines +3 to +8
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>
Copy link
Contributor

@kota2and3kan kota2and3kan Apr 4, 2025

Choose a reason for hiding this comment

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

I have one question. I guess it's related to .NET itself. It's not related to this sample application. However, I'm not familiar with .NET, so I want to ask this question here.

When I tried this sample application in my local environment, I faced the following error.

  • Error

    $ dotnet run LoadInitialData
    /home/ubuntu/tmp/scalardb-samples/scalardb-dotnet-samples/scalardb-cluster-sql-sample/ScalarDbClusterSqlSample.csproj : error NU1101: Unable to find package Microsoft.NETCore.App.Host.ubuntu.22.04-x64. No packages exist with this id in source(s): nuget.org
    
    The build failed. Fix the build errors and run again.
  • Environment information

    $ dotnet --info
    .NET SDK:
     Version:           8.0.114
     Commit:            54ac9c88f1
     Workload version:  8.0.100-manifests.2d3000a2
    
    Runtime Environment:
     OS Name:     ubuntu
     OS Version:  22.04
     OS Platform: Linux
     RID:         ubuntu.22.04-x64
     Base Path:   /usr/lib/dotnet/sdk/8.0.114/
    
    .NET workloads installed:
     Workload version: 8.0.100-manifests.2d3000a2
    There are no installed workloads to display.
    
    Host:
      Version:      8.0.10
      Architecture: x64
      Commit:       81cabf2857
    
    .NET SDKs installed:
      8.0.114 [/usr/lib/dotnet/sdk]
    
    .NET runtimes installed:
      Microsoft.AspNetCore.App 8.0.14 [/usr/lib/dotnet/shared/Microsoft.AspNetCore.App]
      Microsoft.NETCore.App 8.0.14 [/usr/lib/dotnet/shared/Microsoft.NETCore.App]
    
    Other architectures found:
      None
    
    Environment variables:
      DOTNET_ROOT       [/home/ubuntu/.dotnet]
    
    global.json file:
      /home/ubuntu/tmp/scalardb-samples/scalardb-dotnet-samples/scalardb-cluster-sql-sample/global.json
    
    Learn more:
      https://aka.ms/dotnet/info
    
    Download .NET:
      https://aka.ms/dotnet/download

Regarding the above error, I found one issue.
dotnet/sdk#40676 (comment)

And, according to the above issue comment, I fixed the sample app (ScalarDbClusterSqlSample.csproj file) as follows (added <RuntimeIdentifier>linux-x64</RuntimeIdentifier>):

Suggested change
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<RuntimeIdentifier>linux-x64</RuntimeIdentifier>
</PropertyGroup>

After that, I was able to run the sample application in my local environment.

Question 1

It seems that the error above is related to a specific environment (Ubuntu 22.04 + .NET 8.0?). In other words, this error is likely not a general issue with the sample application itself.

Is my understanding correct?

Question 2

As I mentioned in Q1, this error appears to be environment-specific. Therefore, I believe we may not need to make any changes to the sample application itself.

However, it might be beneficial to address this error if we can find a general solution to prevent it.

This is because there is a possibility that some users might run this sample application in an Ubuntu 22.04 + .NET 8.0 environment.

Do you have any ideas on how to resolve this error in a general way?

I apologize, but I am not very familiar with .NET. Therefore, I would like to ask you here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kota2and3kan

Is my understanding correct?

Yes, I think it is. In fact, it seems that that specific environment is Ubuntu 22.04 + .NET 7.0 (or older) + .NET 8.0 (or newer), as it seems that removing all installed .NET versions and then only installing .NET 8.0 fixes the problem.

Do you have any ideas on how to resolve this error in a general way?

Technically, you can set runtime identifiers for all OS's this sample needs to support: <RuntimeIdentifiers>win-x64;win-arm64;win-x86;osx-arm64;linux-x64;linux-arm64</RuntimeIdentifiers>.

But because it’s a problem with .NET/Ubuntu itself and not sample-specific, I don’t think it’s a good idea to “fix” the sample. Maybe it’s better just to add a note to the guides about this error.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dima-X
Thank you for your answer!

Is my understanding correct?

Yes, I think it is. In fact, it seems that that specific environment is Ubuntu 22.04 + .NET 7.0 (or older) + .NET 8.0 (or newer), as it seems that removing all installed .NET versions and then only installing .NET 8.0 fixes the problem.

I see. I understood. I will try the workaround removing all installed .NET versions and then only installing .NET 8.0. Thank you!

Do you have any ideas on how to resolve this error in a general way?

Technically, you can set runtime identifiers for all OS's this sample needs to support: <RuntimeIdentifiers>win-x64;win-arm64;win-x86;osx-arm64;linux-x64;linux-arm64</RuntimeIdentifiers>.

I see. Thank you for the configuration sample!

But because it’s a problem with .NET/Ubuntu itself and not sample-specific, I don’t think it’s a good idea to “fix” the sample. Maybe it’s better just to add a note to the guides about this error.

I agree with you. I think we should add a note about this issue on the documentation side. I will consider it and create a PR on the document side.

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!

Comment on lines +3 to +8
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Dima-X
Thank you for your answer!

Is my understanding correct?

Yes, I think it is. In fact, it seems that that specific environment is Ubuntu 22.04 + .NET 7.0 (or older) + .NET 8.0 (or newer), as it seems that removing all installed .NET versions and then only installing .NET 8.0 fixes the problem.

I see. I understood. I will try the workaround removing all installed .NET versions and then only installing .NET 8.0. Thank you!

Do you have any ideas on how to resolve this error in a general way?

Technically, you can set runtime identifiers for all OS's this sample needs to support: <RuntimeIdentifiers>win-x64;win-arm64;win-x86;osx-arm64;linux-x64;linux-arm64</RuntimeIdentifiers>.

I see. Thank you for the configuration sample!

But because it’s a problem with .NET/Ubuntu itself and not sample-specific, I don’t think it’s a good idea to “fix” the sample. Maybe it’s better just to add a note to the guides about this error.

I agree with you. I think we should add a note about this issue on the documentation side. I will consider it and create a PR on the document side.

@kota2and3kan
Copy link
Contributor

FYI: Regarding the comment #77 (comment), we (@brfrn169 , @komamitsu , and I) discussed and decided to create another PR to add a note to all documents after all related PRs are merged. I will do it later.

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.

LGTM! Thank you!

@feeblefakie
Copy link
Contributor

cc: @josh-wong

@feeblefakie feeblefakie merged commit 977434e into main Apr 8, 2025
32 checks passed
@feeblefakie feeblefakie deleted the add-dotnet-scalardb-cluster-sample branch April 8, 2025 03:02
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.

6 participants