Skip to content
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 the ability to have a separate thread pool/executor for each endpoint #7874

Closed
OmerLitov opened this issue Feb 7, 2021 · 10 comments
Closed

Comments

@OmerLitov
Copy link

a grpc server can have requests of varying priorities.
since a grpc server uses a single executor for all incoming requests, a load of low-priority requests can affect the ability to handle higher priority requests.

The naive approach for this problem would be to have a separate executor for each end-point, but that solution will be a bit limited. In order to have full flexibility, it would be better to have a way to assign an executor based on both the end-point, and the metadata of the request. for example, in our use case, the metadata holds the type of the calling server, and we want to be able to have a dedicated executors for high priority callers.

The solution approach I thought about was adding a new method to the ServerBuilder, that accepts a GrpcExecutorFactory, which is a functional interface with a method that receives (String methodName, Metadata headers), and returns an Executor.

Then, if the GrpcExecutorFactory was set, construct the SerializingExecutor in the ServerImpl with the returned executor.

@OmerLitov
Copy link
Author

@ejona86, please add your suggestions from our discussion.

thanks

@ejona86
Copy link
Member

ejona86 commented Feb 18, 2021

I've been very surprised there's been few users pushing to allow defining an executor per-service or per-method. Basing the decision on metadata is a bit trickier since it requires calling the application (vs application providing configuration data), but the need makes sense.

If making this decision off metadata, it is important the API allow you to protect yourself against "malicious" clients that abuse the "fast lane." That can be done with an interceptor doing an authz-like check to make sure if the client provided this magic header that they are permitted to use that header.

I think we'd want to expose Attributes as well to this API, to allow making decisions based on the client's certificate.

I think there's a few general approaches to this problem:

  1. Allow choosing the executor early in the RPC's lifetime (which is what was suggested by @OmerLitov)
  2. Allow changing the executor after the RPC starts
  3. Provide more information to the executor, to allow it to handle things like this

(1) is more limiting than it sounds, as we use the default executor to look up the ServerMethodDefinition from the fallback registry. ServerMethodDefinition is returned by the ServerCall and the tracers and context aren't fully known, which means the ServerCall hasn't been created yet. So that means we have just a method name and headers, with no per-method configuration data available. We could potentially split this early startup into two parts, basically delaying creating the SerializingExecutor until after the fallback registry returns and thus ServerMethodDefinition is available. This would also allow us to support an asynchronous registry, which has been a requested feature.

(1) would also require calling the user's code from the network thread. We strongly try to avoid that. However, in this case you could view the ExecutorFactory (or whatever it ends up being) as an extension of an Executor and we do call the Executor from the network thread. But not many people implement an Executor, so we'd need some "be careful" documentation in the ExecutorFactory.

(2) is not as crazy as it sounds, by leveraging SerializingExecutor. SerializingExecutor only references the executor if a runnable is not running. The current running thread could change the executor without any synchronization and SerializingExecutor could then hop over to the different executor. By making the executor volatile we could allow the change from any thread. The approach is not as easy with SerializeReentrantCallsDirectExecutor, though. It is unclear how this would be exposed as an API though, especially to make it interceptor-friendly. We could maybe use this approach internally to provide a more powerful 'ExecutorFactory" API that does have access to the ServerCall or to base the decision off ServerMethodDefinition.

(3) is an older idea that originated to allow Executors to fail RPCs when too many Runnables are queued. The normal procedure for that is RejectedExecutionException when execute() is called (which we currently don't support). However, that only allows a tail-drop policy. For a head-drop policy we'd need to expose the ServerCall to the Executor so it could call cancel(), or we could have some specialized API (e.g., pass in two runnables, and calling the second runnable will cancel the call). This approach doesn't entirely line up with the needs of this issue, because the executor would have to make the same decision multiple times. Maybe we could use a "cookie" approach to let it cache its decision; there's a few options.

We currently use ThreadlessExecutor on client-side which is passed into the call via CallOptions. I've been considering for a while what would be necessary for making a blocking server API similarly performant. The most obvious solution here is a ServerMethodDefinition-based API to change the executor, basically making the method definition the server-side equivalent of CallOptions (which aligns with ServerInterceptor2). That said, it is concerning to put an Executor into a lifetime-less object.

@ejona86
Copy link
Member

ejona86 commented Feb 18, 2021

API review meeting votes:

  • option 1: +5
    • Direct from network thread: +1 -2. Several not against, but not a favorite for anyone (two half votes)
    • Call from application thread: +5
  • option 2: -1
  • option 3: +1 (half votes)

So there was clear favoring of option 1 along with reworking SerializingExecutor to delay its creation until after the registry is called so that the ExecutorFactory can be called from the application thread. This would make it easy to allow configuring the Executor from ServerMethodDefinition (at a future date) as well as supporting asynchronous registries. The API could be passed the ServerCall and Metadata. It seems this also satisfies the desires of (3) as the ExecutorFactory was provided the ServerCall and could return an Executor shim that keeps a reference to the ServerCall so it can identify future Runnables.

@ejona86
Copy link
Member

ejona86 commented Feb 18, 2021

We did not discuss a concrete API in the meeting. But a sketch of the API might be like:

// We are close enough to dropping Java 7 support, it is easier to accept
// using interfaces instead of abstract classes.
public interface ExecutorSupplier {
  public Executor getExecutor(ServerCall call, MethodDescriptor headers);
}

public class ServerBuilder {
  // May be a bit confusing since 'executor' is still in use...
  public ServerBuilder executorSupplier(ExecutorSupplier supplier);
}

The supplier is not returning ExecutorServices, so there would be no notification when the Executor is no longer being used. That makes "Factory" poorly suited as a term. I think it is fine not to have any lifecycle associated here; the supplier should have little need to create specific Executors for specific calls. (That's not true for blocking with ThreadlessExecutor; those would be created per-call. But in that case it uses Listener.onComplete/onCancelled to determine its lifetime.)

@ejona86
Copy link
Member

ejona86 commented Feb 18, 2021

We have seen in the past that the Executor can be a source of contention, and fewer Runnables scheduled on it the better. Splitting the registry lookup out to its own Runnable (since the SerializingExecutor wouldn't coalesce them) would introduce more latency because now you must wait for two Runnables, in series. At some point we might want a fast-path where if the executor is not different, then it'd start running callbacks in-line in the same Runnable as used by the registry lookup. A user sensitive to the latency impact but still wanting to use ExecutorSupplier could maybe use directExecutor() to run on the network thread.

@ejona86
Copy link
Member

ejona86 commented Feb 23, 2021

I see three usages of the executor on server-side:

  1. Looking up ServerMethodDefinition (assuming it is split from ServerCall handling)
  2. ServerCall processing
  3. ServerCall cancellation (io.grpc.Context.cancel())

I think we'd either want the ExecutorSupplier to handle all three cases (maybe with additional methods with additional arguments, or maybe just a getExecutor(void)) or carve out a new type of executor, say "call executor", and have the ExecutorSupplier just provide that executor.

@ejona86
Copy link
Member

ejona86 commented Feb 26, 2021

I think I've settled on "split out call executor" into its own "class" of executor. That's the simplest and follows the approach we've taken in the past. So that'd make it look something like:

public interface ServerCallExecutorSupplier {
  public Executor getExecutor(ServerCall call, MethodDescriptor headers);
}
public class ServerBuilder {
  public ServerBuilder callExecutor(ServerCallExecutorSupplier supplier);
}

This approach implies we could have a callExecutor(Executor executor) one day if it was found useful.

@ejona86 ejona86 added this to the Next milestone Feb 26, 2021
@ejona86
Copy link
Member

ejona86 commented Mar 3, 2021

We've gotten a request in Google internally as b/179394103 for per-service override. This can be helpful for the health check or debugging services so they don't have a shared fate (although it depends on the environment whether you want health check to have a shared fate).

@ejona86
Copy link
Member

ejona86 commented Mar 4, 2021

This approach implies we could have a callExecutor(Executor executor) one day if it was found useful.

This would be useful if we introduce a "ServerMethodDefinition-based API to change the executor". If you have a service you want to give their own dedicated threads for reasons like latency, then you probably also want looking up that service in the registry to be separate from normal call execution.

@ejona86
Copy link
Member

ejona86 commented Jun 24, 2021

Fixed by #8266. "ServerMethodDefinition-based API to change the executor" isn't done yet and is still interesting, but isn't techincally part of this issue and users that'd want that API can use the API created here for the while.

@ejona86 ejona86 closed this as completed Jun 24, 2021
@ejona86 ejona86 modified the milestones: Next, 1.40 Jun 24, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants