[Discuss] Use gRPC gateway to generate REST endpoints

[randxie]

Introduction

Right now, feast-java uses RestController (example code) to implement REST endpoints for both feast-core and feast-serving. However, this approach will increase maintenance cost. Therefore, generating REST endpoint from proto definitions is a cleaner way.

Proposed Change

Use grpc-gateway to generate the REST endpoint.

Steps

1. Add gateway server to translate REST call into gRPC call to another process inside the container

The example gateway server will look like:

package main

import (
	"context"
	"flag"
	"fmt"
	"net/http"

	"github.com/golang/glog"
	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
	"google.golang.org/grpc"

	coreGateWay "pkg/gateway/generated/feast/core"
	servingGateWay "pkg/gateway/generated/feast/serving"
)

// gRPC serving endpoint
var servingServerEndpoint = flag.String("serving-server-endpoint", "localhost:6566", "gRPC server endpoint")

func run() error {
	ctx := context.Background()
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()

	mux := runtime.NewServeMux()
	opts := []grpc.DialOption{grpc.WithInsecure()}

	// Register ServingServing endpoint
	err = servingGateWay.RegisterServingServiceHandlerFromEndpoint(ctx, mux, *servingServerEndpoint, opts)
	if err != nil {
		return err
	}

	// Start HTTP server (and proxy calls to gRPC server endpoint)
	return http.ListenAndServe(":8000", mux)
}

func main() {
	flag.Parse()
	defer glog.Flush()

	if err := run(); err != nil {
		fmt.Println(err)
		glog.Fatal(err)
	}
}

2. Use supervisord to start two processes inside a container

We can provide instructions to use different entry points. Here's an example config:

[supervisord]
nodaemon=true

[program:serving]
directory=/
user=root
command=java -jar /opt/feast/feast-serving.jar --spring.config.location=classpath:/application.yml,file:/etc/feast/%(ENV_FEAST_SERVING_YML_NAME)s 

[program:gateway]
directory=/
user=root
command=/opt/feast/gateway_server 
            --serving-server-endpoint=%(ENV_FEAST_SERVING_URL)s
numprocs=1

3. Remove existing REST controllers' code

As titled.

[randxie]

@woop Please feel free to also add other reviewers to the proposal.

[woop]

Thanks @randxie.

I think what would be useful would be discussing the pros and cons of this approach vs sticking to what we have. It seems like what we're saying is

Pros

• Less maintenance for developers
• Less likelihood of drift

Cons

• Harder to start the complete Serving service locally (either need to start two services or build this docker image)
• Need to maintain Go code in the feast-java repo
• Less control over REST semantics
• Need to run two processes in Serving container

Do you agree?

Questions

• Would it be better to start the gateway as a subprocess of the PID 0 process? Doesn't supervisord run in the background? What happens when the Serving service crashes but the gateway is still up?
• Is there a strong reason to build the gateway into the serving service as opposed to leaving it outside (as an external component)?

cc @pyalex

[randxie]

@woop The pros/cons make sense to me. I would like to clarify a few points regarding the proposal:

1. The gRPC gateway server will be created for both feast-core and feast-serving and not just the serving container alone.
2. The gateway server is totally optional as it will be a separate binary running in a separate process (We are serving both gRPC and REST together). So you can still start serving service without creating the gateway server. Also, if you just need gRPC, we can just run the original java binary without running a separate Go gateway server. It's more like a plugin or sidecar.

Regarding the question, please see below:

1. Supervisord can run with nodaemon so it will run in foreground. I think either running in subprocess or a separate process sounds good to me. If the serving service crashes, it will be restarted. As the gateway is still up, the request could timeout.
2. There's no strong reason but I would expect it's easier to get started. For example, if a user specify the entry point as java -jar serving.jar, a single Java process will be run. If the user specifies supervisord -d superviosrd.conf, then both the java binary and the go binary will be run.

To summarize, the Golang server and supervisord config will be added into the container. We can use them if needed. Otherwise, everything will remain the same as before.

[woop]

@woop The pros/cons make sense to me. I would like to clarify a few points regarding the proposal:

1. The gRPC gateway server will be created for both feast-core and feast-serving and not just the serving container alone.

Somehow I missed this. Noted.

Regarding the question, please see below:

1. Supervisord can run with nodaemon so it will run in foreground. I think either running in subprocess or a separate process sounds good to me. If the serving service crashes, it will be restarted. As the gateway is still up, the request could timeout.
2. There's no strong reason but I would expect it's easier to get started. For example, if a user specify the entry point as java -jar serving.jar, a single Java process will be run. If the user specifies supervisord -d superviosrd.conf, then both the java binary and the go binary will be run.

To summarize, the Golang server and supervisord config will be added into the container. We can use them if needed. Otherwise, everything will remain the same as before.

Overall it sounds ok to me. Would like to have some of the others also add their opinions.

[randxie]

@woop Any updates on the review? I can start making the changes if we agree on the proposal.

[woop]

Hey @randxie.

I ran the idea past @khorshuheng and some of the others. Summary

• The current REST methods aren't in use by our clients nor documented. They are safe to remove.
• We do like the idea of having REST if we can have it in a light weight way that doesn't require us to maintain code
• The easiest way to do that is to run a gRPC gateway separately as a standalone container
• There was a concern about what the guarantees would be for us to users that use this REST endpoint. If we do expose and document it as an official endpoint then we can't remove it later
• There was a concern about the added maintenance vs letting teams run the gateway themselves
• There was a concern about performance vs native REST

Since none of those community members have felt strongly enough about the topic to voice concerns, would you be open to the idea of adding the functionality (as proposed) but documenting it as "experimental"? We'll say that REST won't come with the same guarantees as gRPC in terms of stability, compatibility, or performance.

Also, is there any reason we can't use systemd as opposed to supervisord?

[randxie]

@woop Sure, documenting it as experimental makes perfect sense to me. Also, we shouldn't expect REST having same performance as gRPC provides.

Regarding systemd vs supervisord, I don't have any preference. I will explore systemd a bit to see how it works.

[woop]

Great, happy to move forward then. I have a slight preference for systemd, but not major. Just want to keep things simple.