Skip to content

Add REST endpoints for Feast UI#878

Merged
feast-ci-bot merged 25 commits into
feast-dev:masterfrom
yeejian-tan:grpc-http
Aug 17, 2020
Merged

Add REST endpoints for Feast UI#878
feast-ci-bot merged 25 commits into
feast-dev:masterfrom
yeejian-tan:grpc-http

Conversation

@yeejian-tan

@yeejian-tan yeejian-tan commented Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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:

HTTP Endpoint Method Accept Query Parameters?
/api/v1/version GET No
/api/v1/projects GET No
/api/v1/feature-sets GET Yes
/api/v1/features GET Yes
/api/v1/feature-statistics GET Yes

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

The below HTTP endpoints have been added to Feast Core:

| HTTP Endpoint                | Method | Accept Query Parameters? |
|------------------------------|--------|--------------------------|
| `/api/v1/version`            | `GET`  | No                       |
| `/api/v1/projects`           | `GET`  | No                       |
| `/api/v1/feature-sets`       | `GET`  | Yes                      |
| `/api/v1/features`           | `GET`  | Yes                      |
| `/api/v1/feature-statistics` | `GET`  | Yes                      |

For query parameters on specific endpoints, please refer to the javadoc on each
method on the controller.

@yeejian-tan yeejian-tan requested a review from zhilingc as a code owner July 14, 2020 07:41
@Yanson

Yanson commented Jul 14, 2020

Copy link
Copy Markdown
Contributor

Do we really need a rest controller? How about grpc-web or grpc-gateway?

@woop

woop commented Jul 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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?

@Yanson

Yanson commented Jul 15, 2020

Copy link
Copy Markdown
Contributor

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).

woop
woop reviewed Jul 15, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/grpc/CoreServiceResponseGenerator.java Outdated
@woop

woop commented Jul 15, 2020

Copy link
Copy Markdown
Member

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).

Well looking at the code right now it does seem like its way more verbose than I would have liked.

mrzzy
mrzzy reviewed Jul 16, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
mrzzy
mrzzy reviewed Jul 16, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
terryyylim
terryyylim reviewed Jul 17, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
terryyylim
terryyylim reviewed Jul 17, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
@yeejian-tan

yeejian-tan commented Jul 20, 2020

Copy link
Copy Markdown
Collaborator Author

/test test-end-to-end-batch

@yeejian-tan

yeejian-tan commented Jul 20, 2020

Copy link
Copy Markdown
Collaborator Author

/test test-end-to-end-auth

@yeejian-tan

yeejian-tan commented Jul 20, 2020

Copy link
Copy Markdown
Collaborator Author

/test test-end-to-end-batch-dataflow

woop
woop reviewed Jul 20, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
woop
woop reviewed Jul 20, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
woop
woop reviewed Jul 20, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
woop
woop reviewed Jul 20, 2020
View reviewed changes
Comment thread core/src/main/java/feast/core/controller/CoreServiceRestController.java Outdated
@yeejian-tan yeejian-tan force-pushed the grpc-http branch 4 times, most recently from 6da46db to f666287 Compare July 23, 2020 04:56
@yeejian-tan
Make format-java
489c9ae
@yeejian-tan
Make feature_set_id non-required per gRPC
6d19fe6
@yeejian-tan
Add Unit Tests for Core REST Controller
bddf539
@yeejian-tan
Add javadoc, remove json printer
1f7f1d9
@yeejian-tan
Fix Javadocs Errors and typo
7f73645
@yeejian-tan
Modify Unit tests to not rely on Mockmvc
d77a2ca
@yeejian-tan
Add Integration Tests for CoreServiceRestController
762b6ef
@yeejian-tan
Remove labels as it is not yet fully tested
4ab312c
@yeejian-tan
Remove getFeatureSet REST endpoint
ef99493 
Since it is a subset of the listFeatureSet endpoint.
@yeejian-tan
Add integration tests for REST controller
9dc772b
@yeejian-tan
Remove default query params for grpc consistency
901dbe8
@yeejian-tan
Remove Unit Tests
8e23674
@yeejian-tan
Keep get feature-statistics consistent with grpc
1c9efd4
@yeejian-tan
Add comments on exception handlers for core http
f5bae5d
@yeejian-tan
Use WebTestClient for core HTTP ITs
2169c0a
@yeejian-tan
Use reactor-test and remove autowired params
f545a2c
@yeejian-tan
Use RestAssured for REST IT
7ce74e0
@yeejian-tan
Replace ProjectService w/ AccessManagementService
1ba9a40
@yeejian-tan
Handle retrieval errors caused by getFeatureStats
9dab69f
@yeejian-tan
Add disableRestControllerAuth flag
6e8c8df
@yeejian-tan
Add documentation comment on application.yml
4156a4f
@feast-ci-bot feast-ci-bot removed the lgtm label Aug 17, 2020
@terryyylim

terryyylim commented Aug 17, 2020

Copy link
Copy Markdown
Member

/test test-end-to-end-batch-dataflow

@terryyylim

terryyylim commented Aug 17, 2020

Copy link
Copy Markdown
Member

/test test-end-to-end-batch

1 similar comment
@terryyylim

terryyylim commented Aug 17, 2020

Copy link
Copy Markdown
Member

/test test-end-to-end-batch

@woop

woop commented Aug 17, 2020

Copy link
Copy Markdown
Member

/lgtm

@feast-ci-bot feast-ci-bot merged commit 7d8126b into feast-dev:master Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrzzy mrzzy mrzzy left review comments

@woop woop woop approved these changes

@zhilingc zhilingc Awaiting requested review from zhilingc

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@pyalex pyalex Awaiting requested review from pyalex

@dr3s dr3s Awaiting requested review from dr3s

@terryyylim terryyylim Awaiting requested review from terryyylim

Assignees

@woop woop

Labels

approved kind/feature New feature or request lgtm size/XL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yeejian-tan @Yanson @woop @mrzzy @dr3s @terryyylim @feast-ci-bot