-
Notifications
You must be signed in to change notification settings - Fork 671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update FlightSqlService
trait to proxy handshake
#2211
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,16 @@ pub trait FlightSqlService: | |
/// When impl FlightSqlService, you can always set FlightService to Self | ||
type FlightService: FlightService; | ||
|
||
/// Accept authentication and return a token | ||
/// <https://arrow.apache.org/docs/format/Flight.html#authentication> | ||
async fn do_handshake( | ||
&self, | ||
request: Request<Streaming<HandshakeRequest>>, | ||
) -> Result< | ||
Response<Pin<Box<dyn Stream<Item = Result<HandshakeResponse, Status>> + Send>>>, | ||
Status, | ||
>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we provide default implementation that returns "Not yet implemented" error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still learning Rust. Would this be done via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wow, I just threw the default implementation right on the trait and it compiled! I had no idea that would work nowadays 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, as you see, you can "override" default implementation in a trait nowadays. |
||
|
||
/// Get a FlightInfo for executing a SQL query. | ||
async fn get_flight_info_statement( | ||
&self, | ||
|
@@ -256,9 +266,10 @@ where | |
|
||
async fn handshake( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this isn't cool that the |
||
&self, | ||
_request: Request<Streaming<HandshakeRequest>>, | ||
request: Request<Streaming<HandshakeRequest>>, | ||
) -> Result<Response<Self::HandshakeStream>, Status> { | ||
Err(Status::unimplemented("Not yet implemented")) | ||
let res = self.do_handshake(request).await?; | ||
Ok(res) | ||
} | ||
|
||
async fn list_flights( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
It would be awesome to get some test coverage of this code eventually 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to follow acceptance test driven development when possible. Do you think the rust FlightSqlClient is far enough along to integration test with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that we eventually should add some test coverage for flight sql server. Currently we don't have any test coverage for it, not just this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we have TPC-H up and running, I'll come back around to this and look at the main issues: cloning the server state & testing. Thanks for your feedback and helping get it merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is also a place for targeted for regression tests (so that, for example, if someone breaks the code accidentally in some future refactoring, they also get a test failure). However, the right balance and where to draw the line between the two is always a matter of judgement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't mean to imply integration tests were the only way. I think they become exponentially impossible to maintain as code coverage increases - so unit tests are also required. I was just (and still am) hoping to start with a FlighSql-rs test then add unit tests in addition to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we are in total agreement