GH-46315 [C#] Apache Arrow Flight Middleware #46316
Conversation
…ing headers and cookie values - Implemented tests verifying that CookieMiddleware correctly captures and persists values from both headers and cookies. - Added CapturingCookieMiddleware and CapturingCookieMiddlewareFactory for validating middleware lifecycle. - Ensured middleware correctly processes cookie strings and validates persistence throughout the request lifecycle.
…s it will add up and force more GC calls)
|
|
…nto feat/flight-csharp-middleware # Conflicts: # csharp/Directory.Build.props # csharp/src/Apache.Arrow.Flight/Middleware/ClientCookieMiddleware.cs # csharp/src/Apache.Arrow.Flight/Middleware/ClientCookieMiddlewareFactory.cs # csharp/src/Apache.Arrow.Flight/Middleware/CookieManager.cs # csharp/src/Apache.Arrow.Flight/Middleware/Extensions/FlightMethodParser.cs # csharp/src/Apache.Arrow.Flight/Middleware/Extensions/StatusUtils.cs # csharp/src/Apache.Arrow.Flight/Middleware/Interceptors/ClientInterceptorAdapter.cs # csharp/test/Apache.Arrow.Flight.Tests/MiddlewareTests/ClientCookieMiddlewareTests.cs # csharp/test/Apache.Arrow.Flight.Tests/MiddlewareTests/Stubs/ClientCookieMiddlewareMock.cs
…r for cookie middleware - Remove unused and redundant files
|
@HackPoint can you please rebase this change now that the other change is checked-in? |
Alternatively, is it okay if we fork the C# implementation to a separate repo now? Rebasing this change might be complicated enough that we get no real benefit from continuing to use the same repo and this change is the last "pending" one before the proposed fork. |
Than what's your preferred thing to do? Should I continue with merge and rebase or not? |
Merge and rebase is fine; I wasn't sure if you were working on this. |
Sir, I was so glad that you merged the pr , I started the moment you have mentioned this rebase. I almost done and started testing it against our db. The moment I will finish, I'll push the update. |
…ddleware # Conflicts: # csharp/src/Apache.Arrow.Flight.Sql/Client/FlightSqlClient.cs # csharp/src/Apache.Arrow.Flight.Sql/DoPutResult.cs # csharp/src/Apache.Arrow.Flight.Sql/PreparedStatement.cs # csharp/src/Apache.Arrow.Flight.Sql/SqlActions.cs # csharp/src/Apache.Arrow.Flight.Sql/Transaction.cs # csharp/test/Apache.Arrow.Flight.Sql.Tests/FlightSqlClientTests.cs
|
@CurtHagenlocher Hi sir, it's completed and all the integration has been done and tested. Please merge it it should be working fine. |
|
|
|
||
| namespace Apache.Arrow.Flight.Middleware; | ||
|
|
||
| public class MetadataAdapter : ICallHeaders |
There was a problem hiding this comment.
This class doesn't appear to be used in this PR. Is it possible it's been superseded by CallHeaders?
| } | ||
| catch (FormatException ex) | ||
| { | ||
|
|
There was a problem hiding this comment.
nit: remove blank lines here and at line 69
| { | ||
| outgoingHeaders.Insert(COOKIE_HEADER, cookieValue); | ||
| } | ||
| _logger.LogInformation("Sending Headers: " + string.Join(", ", outgoingHeaders)); |
There was a problem hiding this comment.
What does this log? It doesn't look like any of the types implementing ICallHeaders have overridden ToString. And given that the cookies might include credential information, it's probably not a good idea to log all their values.
Same comment applies to line 53.
| using Apache.Arrow.Flight.Middleware.Extensions; | ||
| using Apache.Arrow.Flight.Middleware.Interfaces; | ||
| using Microsoft.Extensions.Logging; | ||
| namespace Apache.Arrow.Flight.Middleware; |
There was a problem hiding this comment.
nit: insert a blank line before the namespace declaration
|
|
||
| public ClientCookieMiddlewareFactory(ILoggerFactory loggerFactory) | ||
| { | ||
| _loggerFactory = loggerFactory; |
There was a problem hiding this comment.
Wouldn't it make more sense to create and hold on to a single ILogger instance instead of creating a new one for every call?
| m?.OnHeadersReceived(new CallHeaders(headers)); | ||
| } | ||
|
|
||
| return task.Result; |
There was a problem hiding this comment.
If task.Exception can be not-null, then should this throw the exception instead of returning null?
| { | ||
| var headers = task.Result; | ||
| foreach (var m in middlewares) | ||
| m?.OnHeadersReceived(new CallHeaders(headers)); |
There was a problem hiding this comment.
Can the new CallHeaders(headers) be lifted out of the loop to save allocations?
| var headers = await headersTask.ConfigureAwait(false); | ||
| foreach (var m in middlewares) | ||
| { | ||
| m?.OnHeadersReceived(new CallHeaders(headers)); |
There was a problem hiding this comment.
Can the new CallHeaders(headers) be lifted out of the loop to save allocations?
| var response = await responseTask.ConfigureAwait(false); | ||
| foreach (var m in middlewares) | ||
| { | ||
| m?.OnCallCompleted(getStatus(), getTrailers()); |
There was a problem hiding this comment.
Can the getStatus() and getTrailers() be lifted out of the loop instead of being called repeatedly? If necessary, this could be e.g. var status = middlewares.Count > 0 ? getStatus() : default to avoid the calls when no middleware is present.
| } | ||
| catch | ||
| { | ||
| foreach (var m in middlewares) |
There was a problem hiding this comment.
If the very last call to OnCallCompleted throws an exception, we're going to cycle through the list a second time calling all the middleware again. Consider instead catching errors inside of the enumeration and then only bubbling the exception out at the very end -- perhaps wrapping in an AggregateException if there is more than one.
github-actions
bot
added
awaiting committer review
Enhancement: Apache Arrow Flight Middleware in C#
Overview
This Pull Request enhances middleware support for Apache Arrow Flight using C#, focusing on improved metadata header management and propagation for better observability and extensibility. It also provides handling for HTTP/HTTPS communication.
Rationale for this Change
Effective middleware is critical for managing metadata headers, ensuring accurate request/response handling, and simplifying debugging in distributed systems. By improving middleware capabilities, we enhance reliability and observability, significantly benefiting developers and operational teams managing complex Flight-based applications.
What's Included in this PR?
Key Features
OnBeforeSendingHeaders,OnHeadersReceived,OnCallCompleted).Impact
Are These Changes Tested?
Testing Overview
Unit Tests:
OnBeforeSendingHeaders,OnHeadersReceived,OnCallCompleted).Integration Tests:
x-server-header,Set-Cookie) between client and server.End-to-End Tests:
Example Test Cases:
OnHeadersReceivedcorrectly captures server-sent headers.OnCallCompletedis triggered on both success and error cases.Checklist