feat(csharp): logging enhancements#28
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves observability and metadata retrieval behavior by adding Activity-based tracing around HiveServer2 metadata operations, parallelizing parts of GetColumnsExtendedAsync, and fixing a logging serialization issue for Thrift status codes.
Changes:
- Wrap multiple metadata methods with
TraceActivityAsync, adding events/tags for richer tracing. - Run
GetColumnsAsync,GetPrimaryKeysAsync, and FK lookup in parallel insideGetColumnsExtendedAsync. - Store Thrift
StatusCodeas aninttag value to avoid serialization failures.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
csharp/src/Hive2/HiveServer2Statement.cs |
Adds tracing + parallelizes extended columns metadata retrieval and optimizes relationship processing. |
csharp/src/Hive2/HiveServer2Connection.cs |
Fixes Activity tag serialization by casting StatusCode to int. |
csharp/arrow-adbc |
Updates the Arrow ADBC submodule pointer. |
csharp/Directory.Packages.props |
Bumps Microsoft.NET.Test.Sdk version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 1. Launch all three independent metadata calls in parallel | ||
| activity?.AddEvent("hive2.statement.get_columns_extended.launching_parallel_calls"); | ||
| var columnsTask = GetColumnsAsync(cancellationToken); | ||
| var pkTask = GetPrimaryKeysAsync(cancellationToken); | ||
| // For FK lookup, we need to pass in the current catalog/schema/table as the foreign table | ||
| var fkTask = GetCrossReferenceAsForeignTableAsync(cancellationToken); | ||
|
|
||
| await Task.WhenAll(columnsTask, pkTask, fkTask).ConfigureAwait(false); |
There was a problem hiding this comment.
Running these metadata calls in parallel can concurrently use the same underlying Connection / Thrift client. If the Thrift client or connection pipeline is not explicitly thread-safe for concurrent requests, this can cause intermittent protocol errors or corrupted responses. A robust approach is to enforce a connection-level async lock (e.g., SemaphoreSlim) around Thrift invocations, or to make parallelism conditional (feature flag / capability check) so callers can opt out when using non-concurrent transports.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Bruce Irschick <bruce.irschick@improving.com>
|
I was adding a test for |
|
Well, that's disappointing. I recall the issue, but I didn't face it in my testing I was doing. I guess I need to remove the Task.WhenAll logic. |
What's Changed