Skip to content

Add .NET Sdk#187

Open
sp-jcberleur wants to merge 9 commits into
databricks:mainfrom
sp-jcberleur:feat/dotnet
Open

Add .NET Sdk#187
sp-jcberleur wants to merge 9 commits into
databricks:mainfrom
sp-jcberleur:feat/dotnet

Conversation

@sp-jcberleur

Copy link
Copy Markdown

What changes are proposed in this pull request?

Initial Databricks Zerobus Ingest SDK for .NET to ingest data from .NET applications

How is this tested?

dotnet test

@teodordelibasic-db teodordelibasic-db left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for contributing and sorry for the delayed review, we've been quite overwhelmed with product stabilization these past few weeks. I'll prioritize now so that we can get it in, a few other customers also want to use it. I am nowhere near a C#/.NET expert so I am reviewing mostly with LLM help.

A few more general comments below as well:


We should integrate the .NET CI into general repo CI. Currently PRs touching only dotnet/** pass CI without any checks and Rust FFI changes don't trigger cross-SDK .NET tests. In .github/workflows/push.yml we should add:

  1. dotnet output in the changes job.
  2. dotnet path filter (dotnet/** + .github/workflows/ci-dotnet.yml)
  3. dotnet job calling ci-dotnet.yml
  4. dotnet in the gate job's needs array + result check loop
  5. cross-sdk-dotnet job (mirroring cross-sdk-go) triggered when rust/** changes

A couple of missing misc files in the dotnet/ directory that we have in other SDKs:

  1. CONTRIBUTING.md - guidelines how to setup a devloop for making changes and contributing them for this SDK.
  2. NEXT_CHANGELOG.md - used by the release process for unreleased changes
  3. NOTICE - license/attribution file

As far as I understand, this is a sync-only API, LLM suggests we could benefit from an API returning Task object for methods CreateStream, WaitForOffset, Flush, and Close. ASP.NET Core users calling current API on request threads will block thread pool threads.

Not a blocker obviously, just want to cross-check with you if it makes sense to add it in the future.

Comment thread dotnet/src/Zerobus/Native/HeadersProviderBridge.cs Outdated
Comment thread dotnet/src/Zerobus/Native/HeadersProviderBridge.cs Outdated
Comment thread dotnet/src/Zerobus/Native/HeadersProviderBridge.cs Outdated
Comment thread dotnet/src/Zerobus/ZerobusStream.cs
Comment thread dotnet/src/Zerobus/Native/NativeInterop.cs Outdated
Comment thread dotnet/src/Zerobus/Native/NativeInterop.cs Outdated
Comment thread dotnet/src/Zerobus/Native/NativeInterop.cs Outdated

return new CStreamConfigurationOptions
{
MaxInflightRequests = (nuint)(options.MaxInflightRequests > 0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Setting RecoveryRetries = 0 (meaning "don't retry") silently falls back to the default of 4. The Go SDK has the same bug (comment at go/ffi.go:193). We can add a code comment acknowledging this or use nullable types (uint?) to distinguish "unset" from "explicitly zero".

Comment thread dotnet/src/Zerobus/ZerobusSdk.cs
@RichardCupples

Copy link
Copy Markdown

Any progress, PR has been parked for 2 months now?

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.

3 participants