CurtHagenlocher commented on code in PR #43910:
URL: https://github.com/apache/arrow/pull/43910#discussion_r1741120117
##########
csharp/src/Apache.Arrow.Flight/Client/FlightClient.cs:
##########
@@ -32,35 +32,35 @@ public FlightClient(ChannelBase grpcChannel)
_client = new FlightService.FlightServiceClient(grpcChannel);
}
- public AsyncServerStreamingCall<FlightInfo> ListFlights(FlightCriteria
criteria = null, Metadata headers = null)
+ public AsyncServerStreamingCall<FlightInfo> ListFlights(FlightCriteria
criteria = null, Metadata headers = null, System.DateTime? deadline = null,
System.Threading.CancellationToken cancellationToken =
default(global::System.Threading.CancellationToken))
Review Comment:
nit: consider `using System.Threading` unless it creates a conflict, and
`default` instead of `default(CancellationToken)`.
##########
csharp/src/Apache.Arrow.Flight/Client/FlightClient.cs:
##########
@@ -32,35 +32,35 @@ public FlightClient(ChannelBase grpcChannel)
_client = new FlightService.FlightServiceClient(grpcChannel);
}
Review Comment:
I know it's kind of annoying, but can you add these as overloads instead of
changing the original definition? e.g
```
public AsyncServerStreamingCall<FlightInfo> ListFlights(FlightCriteria
criteria = null, Metadata headers = null) {
return ListFlights(criteria, headers, null, CancellationToken.None)
}
public AsyncServerStreamingCall<FlightInfo> ListFlights(FlightCriteria
criteria, Metadata headers, System.DateTime? deadline, CancellationToken
cancellationToken = default) {
// ...
}
```
That way, binary compatibility is preserved between versions, which matters
under some circumstances.
##########
csharp/test/Apache.Arrow.Flight.Tests/FlightTests.cs:
##########
@@ -415,5 +418,65 @@ public async Task
EnsureTheSerializedBatchContainsTheProperTotalRecordsAndTotalB
Assert.Equal(expectedBatch.Length, result.TotalRecords);
Assert.Equal(expectedTotalBytes, result.TotalBytes);
}
+
+ [Fact]
+ public async Task EnsureCallRaiseDeadlineExceeded()
Review Comment:
super nit: can these be named `CallRaises` instead of `CallRaise`?
--
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: github-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
