ARROW-13225: [Go][FlightRPC][Integration] Implement Flight Custom Middleware and Integration Tests for Go #10633
Conversation
|
Tagging @sbinet @emkornfield @nickpoorman for visibility, please add any other relevant people to this, thanks! I ended up pursuing this when I was trying to add opentracing to our flight client and server and realized that it was pretty annoying and difficult to actually utilize the interceptors to do so given the current configuration. So this should make it much more convenient. |
| +-----------------------------+-------+-------+-------+------------+-------+-------+-------+ | ||
| | Authentication handlers | ✓ | ✓ | ✓ | | ✓ (2) | | | | ||
| +-----------------------------+-------+-------+-------+------------+-------+-------+-------+ | ||
| | Custom client middleware | ✓ | ✓ | | | | | | | ||
| | Custom client middleware | ✓ | ✓ | ✓ | | | | | |
There was a problem hiding this comment.
i think this was brought up no the mailing list. For new feature we might want to start recording verson numbers that a feature was availalable int (or maybe add this as a foot-note.
There was a problem hiding this comment.
That's gonna be a bit difficult until the CI is updated to create the necessary tags for the golang libraries, I believe there's a JIRA card for it, I just don't know enough about that side of the CI scripts to know what to change and where in order to add the correct formatted tags for the go versions
| @@ -63,6 +104,8 @@ type server struct { | |||
| // Alternatively, a grpc server can be created normally without this helper as the | |||
| // grpc server generated code is still being exported. This only exists to allow | |||
| // the utility of the helpers | |||
| // | |||
| // Deprecated: prefer to use NewServerWithMiddleware | |||
There was a problem hiding this comment.
should this be deprecated, if implementators don't care about middleware, is there a strong reason to use that method over this one?
There was a problem hiding this comment.
There's a couple reasons:
The NewServer function can actually cause problems if using an auth handler + passing in grpc options that set interceptors because of how it was handled, whereas using this given the definition of the middlewares makes it explicit that interceptors should be set using the middleware rather than using the generic grpc options.
In addition, by marking this deprecated, we can eventually remove this old function and make NewServerWithMiddleware the actual NewServer function so everything is very explicit.
|
Skimmed through, generally looks OK to me. CC @lidavidm in case you want to take a look. |
|
@emkornfield @lidavidm Any luck on getting this merged soon? |
|
Hey, sorry for the delay - I'm not super familiar with Go so if Micah is happy with it and the integration tests pass then it's good to me. I'll merge it later today in case Micah has anything else to chime in with. |
No description provided.