-
Notifications
You must be signed in to change notification settings - Fork 998
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
Add REST endpoints for Feast UI #878
Conversation
Do we really need a rest controller? How about grpc-web or grpc-gateway? |
@Yanson I am a little bit conflicted myself on which approach to take. I originally asked @SwampertX to use grpc-gateway, but after some discussion we decided to implement this as a REST controller. The biggest reason being that we didn't want to maintain another proxy and keep it in sync with our APIs, and I wasn't sure how well a gRPC proxy would play with our auth setup. I dont feel too strongly either way though. Right now the goal is just to get a REST endpoint up so that we can continue development on the UI, however that is done. Is your concern around code maintenance or dependencies? |
I'm not very opinionated on this as I haven't used either of my suggestions. It just seems like we might be creating work for ourselves and require maintaining parity with two sets of APIs. I thought grpc-gateway was more automatic than you suggest, but my inclination would still have been to see if grpc-web gets us what we need (again, never used it myself). |
core/src/main/java/feast/core/grpc/CoreServiceResponseGenerator.java
Outdated
Show resolved
Hide resolved
Well looking at the code right now it does seem like its way more verbose than I would have liked. |
core/src/main/java/feast/core/controller/CoreServiceRestController.java
Outdated
Show resolved
Hide resolved
core/src/main/java/feast/core/controller/CoreServiceRestController.java
Outdated
Show resolved
Hide resolved
...rc/main/java/feast/core/controller/exception/handler/RestResponseEntityExceptionHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/feast/core/controller/CoreServiceRestController.java
Outdated
Show resolved
Hide resolved
/test test-end-to-end-batch |
/test test-end-to-end-auth |
/test test-end-to-end-batch-dataflow |
core/src/main/java/feast/core/controller/CoreServiceRestController.java
Outdated
Show resolved
Hide resolved
core/src/main/java/feast/core/controller/CoreServiceRestController.java
Outdated
Show resolved
Hide resolved
core/src/main/java/feast/core/controller/CoreServiceRestController.java
Outdated
Show resolved
Hide resolved
6da46db
to
f666287
Compare
Since it is a subset of the listFeatureSet endpoint.
/test test-end-to-end-batch-dataflow |
/test test-end-to-end-batch |
1 similar comment
/test test-end-to-end-batch |
/lgtm |
What this PR does / why we need it: In preparation for Feast UI (#834), we
need REST endpoints for Feast UI to consume. This PR creates such endpoints.
The below HTTP endpoints have been added to Feast Core:
/api/v1/version
GET
/api/v1/projects
GET
/api/v1/feature-sets
GET
/api/v1/features
GET
/api/v1/feature-statistics
GET
For query parameters on specific endpoints, please refer to the javadoc on each
method on the controller.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?:
Yes