NCO-57: Async Analytics Query#59
Conversation
The spec changed a bit, so we refactored our existing unreleased code accordingly. - Add new QueryStatus class as an intermediate between QueryHandle and QueryResultHandle, replacing the nullable QueryResultHandle? polling pattern with ResultsReady/ResultHandle() per EA-4 spec - Rename FetchResultHandleAsync to FetchStatusAsync across QueryHandle, IAnalyticsService, and AnalyticsService - Rename FetchResultHandleOptions to FetchStatusOptions - Make QueryHandle.Handle and QueryHandle.RequestId internal - Add ProcessedObjects to AsyncQueryMetrics - Enrich QueryStatus.ToString() with all available server response fields (resultCount, resultSetOrdered, partitions, createdAt, metrics) with resilient handling when fields are absent - Update all unit tests, functional tests, and test helpers
There was a problem hiding this comment.
Pull request overview
Refactors the async analytics query workflow to match the updated EA-4 spec by introducing a QueryStatus intermediate type, renaming the polling API from “result handle” to “status”, and updating result/streaming behaviors plus related tests.
Changes:
- Introduces
QueryStatuswithResultsReady+ResultHandle()and richerToString()diagnostics. - Renames
FetchResultHandleAsync→FetchStatusAsync(and associated options types) across service/handles and updates tests accordingly. - Adds async disposal support for query results and adjusts streaming enumeration behavior (including new unit tests).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Couchbase.Analytics.UnitTests/Internal/StreamingAnalyticsResultTests.cs | Adds coverage for single-enumeration behavior and async disposal. |
| tests/Couchbase.Analytics.UnitTests/Internal/ExecuteQueryTests.cs | Updates test shim to implement IAsyncDisposable on IQueryResult. |
| tests/Couchbase.Analytics.UnitTests/Internal/AnalyticsServiceTests.cs | Renames test to FetchStatusAsync and updates options types. |
| tests/Couchbase.Analytics.UnitTests/Helpers/TestHandleFactory.cs | Adds CreateQueryStatus helper for new async status type. |
| tests/Couchbase.Analytics.UnitTests/Async/QueryStatusTests.cs | New unit tests validating QueryStatus parsing, readiness, and ToString(). |
| tests/Couchbase.Analytics.UnitTests/Async/QueryHandleTests.cs | Updates handle tests to delegate via FetchStatusAsync. |
| tests/Couchbase.Analytics.FunctionalTests/Internal/CouchbaseHttpClientTests.cs | Updates result iteration to use result.Rows. |
| tests/Couchbase.Analytics.FunctionalTests/Internal/AnalyticsServiceTests.cs | Updates result iteration to use result.Rows. |
| tests/Couchbase.Analytics.FunctionalTests/AsyncAnalyticsTests.cs | Updates end-to-end async polling to use QueryStatus + ResultsReady. |
| src/Couchbase.Analytics/Results/IQueryResult.cs | Extends IQueryResult with IAsyncDisposable. |
| src/Couchbase.Analytics/Query/AsyncQueryMetrics.cs | Adds ProcessedObjects for async-query metrics deserialization. |
| src/Couchbase.Analytics/Options/FetchStatusOptions.cs | Renames options record and updates docs to “status”. |
| src/Couchbase.Analytics/Internal/Results/StreamingAnalyticsResult.cs | Changes second enumeration behavior to throw an exception. |
| src/Couchbase.Analytics/Internal/Results/AnalyticsResultBase.cs | Adds DisposeAsync() implementation for results. |
| src/Couchbase.Analytics/Internal/IAnalyticsService.cs | Renames service polling method to FetchStatusAsync returning QueryStatus. |
| src/Couchbase.Analytics/Internal/AnalyticsService.cs | Implements FetchStatusAsync, clones parsed JSON, updates logging and error handling. |
| src/Couchbase.Analytics/Async/QueryStatus.cs | Adds new status type with parsing, readiness logic, and diagnostics. |
| src/Couchbase.Analytics/Async/QueryHandle.cs | Makes handle/requestId internal and renames polling API to FetchStatusAsync. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot review |
- result disposal safer - thread-safe enumeration guard throughout - non-JSON responses handled without exception
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/Couchbase.Analytics/Internal/Results/StreamingAnalyticsResult.cs:76
EnumerateRowsmarks the result as already enumerated before verifying initialization (_hasReadToResult). If a consumer accidentally enumerates before callingInitializeAsync, this will permanently flip_enumeratedand subsequent (correct) enumeration after initialization will always throw. Consider moving theInterlocked.CompareExchangecheck to after the initialization guard so a pre-initialize call fails without consuming the one-time enumeration slot.
if (Interlocked.CompareExchange(ref _enumerated, 1, 0) != 0)
{
throw new InvalidOperationException(
"Query results can only be enumerated once. The result stream has already been consumed.");
}
if (!_hasReadToResult)
{
throw new InvalidOperationException(
$"{nameof(StreamingAnalyticsResult)} has not been initialized, call InitializeAsync first");
}
src/Couchbase.Analytics/Internal/AnalyticsService.cs:344
FetchStatusAsynccreates a short-lived HttpClient viaCreateHttpClient(timeout)but never disposes it.ICouchbaseHttpClientFactory.Create()is documented as producing a client intended to be disposed after use; polling this endpoint can create many clients and increase memory/socket pressure. Dispose the HttpClient in this method (e.g., viausing/await usingor atry/finally).
var timeout = _clusterOptions.TimeoutOptions.DispatchTimeout;
var httpClient = CreateHttpClient(timeout);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…hods Fix enumeration guard ordering in StreamingAnalyticsResult
The spec changed a bit, so we refactored our existing unreleased code accordingly.