CurtHagenlocher commented on code in PR #285:
URL: https://github.com/apache/arrow-dotnet/pull/285#discussion_r3004957340
##########
src/Apache.Arrow.Flight/Client/FlightClient.cs:
##########
@@ -27,15 +27,18 @@ public class FlightClient
internal static readonly Empty EmptyInstance = new Empty();
private readonly FlightService.FlightServiceClient _client;
+ private readonly ArrowContext _context;
- public FlightClient(ChannelBase grpcChannel)
+ public FlightClient(ChannelBase grpcChannel, ArrowContext context =
null)
{
_client = new FlightService.FlightServiceClient(grpcChannel);
+ _context = context;
}
- public FlightClient(CallInvoker callInvoker)
+ public FlightClient(CallInvoker callInvoker, ArrowContext context =
null)
Review Comment:
It would then also be good to add a test which exercises this code path. You
could clone the existing `TestIntegrationWithGrpcNetClientFactory` test and
then replace the client setup with
```
services.AddGrpcClient<FlightClient>(grpc => grpc.Address = new
Uri(_testWebFactory.GetAddress()))
.ConfigureGrpcClientCreator(invoker =>
{
return FlightClient.Create(invoker, new ArrowContext());
});
```
This will also demonstrate to users how to use an `ArrowContext` with the
factory.
##########
src/Apache.Arrow.Flight/Client/FlightClient.cs:
##########
@@ -27,15 +27,18 @@ public class FlightClient
internal static readonly Empty EmptyInstance = new Empty();
private readonly FlightService.FlightServiceClient _client;
+ private readonly ArrowContext _context;
- public FlightClient(ChannelBase grpcChannel)
+ public FlightClient(ChannelBase grpcChannel, ArrowContext context =
null)
Review Comment:
For binary backwards compatibility, can you instead add a second public
constructor with the additional parameter? i.e.
```
public FlightClient(ChannelBase grpcChannel) : this(grpcChannel. null)
{
}
public FlightClient(ChannelBase grpcChannel, ArrowContext context)
{
_client = new FlightService.FlightServiceClient(grpcChannel);
_context = context;
}
```
##########
src/Apache.Arrow.Flight/Server/FlightServerRecordBatchStreamReader.cs:
##########
@@ -21,11 +21,11 @@ namespace Apache.Arrow.Flight.Server
{
public class FlightServerRecordBatchStreamReader :
FlightRecordBatchStreamReader
{
- public
FlightServerRecordBatchStreamReader(IAsyncStreamReader<FlightData>
flightDataStream) : base(new StreamReader<FlightData,
Protocol.FlightData>(flightDataStream, data => data.ToProtocol()))
+ public
FlightServerRecordBatchStreamReader(IAsyncStreamReader<FlightData>
flightDataStream, ArrowContext context = null) : base(new
StreamReader<FlightData, Protocol.FlightData>(flightDataStream, data =>
data.ToProtocol()), context)
Review Comment:
For binary backwards compatibility, can you instead add a second public
constructor with the additional parameter? i.e.
```
public FlightServerRecordBatchStreamReader(IAsyncStreamReader<FlightData>
flightDataStream)
: this(flightDataStream, null)
{
}
public FlightServerRecordBatchStreamReader(IAsyncStreamReader<FlightData>
flightDataStream, ArrowContext context)
: base(new StreamReader<FlightData,
Protocol.FlightData>(flightDataStream, data => data.ToProtocol()), context)
{
}
```
##########
src/Apache.Arrow.Flight/Client/FlightClient.cs:
##########
@@ -27,15 +27,18 @@ public class FlightClient
internal static readonly Empty EmptyInstance = new Empty();
private readonly FlightService.FlightServiceClient _client;
+ private readonly ArrowContext _context;
- public FlightClient(ChannelBase grpcChannel)
+ public FlightClient(ChannelBase grpcChannel, ArrowContext context =
null)
{
_client = new FlightService.FlightServiceClient(grpcChannel);
+ _context = context;
}
- public FlightClient(CallInvoker callInvoker)
+ public FlightClient(CallInvoker callInvoker, ArrowContext context =
null)
Review Comment:
For binary backwards compatibility, can you instead add a second constructor
with the additional parameter? This one is trickier because the DI mechanism
doesn't like having two constructors that both match what it's looking for, so
we need to use a factory method instead:
```
public FlightClient(CallInvoker callInvoker) : this(callInvoker. null)
{
}
private FlightClient(CallInvoker callInvoker, ArrowContext context)
{
_client = new FlightService.FlightServiceClient(callInvoker);
_context = context;
}
public static FlightClient Create(CallInvoker callInvoker, ArrowContext
context)
{
return new FlightClient(callInvoker, context);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
