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

[server] Rewrite gRPC read Service to align with Netty-Based HTTP Server #1163

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

sushantmane
Copy link
Contributor

@sushantmane sushantmane commented Sep 9, 2024

Rewrite gRPC read Service to align with Netty-Based HTTP Server

  • Refactored the Netty handler to enable code reuse for gRPC handlers
  • Added missing APIs to the gRPC service to close feature gaps with the
    HTTP implementation.
  • Fixed the gRPC I/O request path threading model: previously, requests
    were handled by the gRPC executor thread pool, but now they are processed
    by the StorageReadExecutor thread pool for better performance.
    • Deprecated the old get and multiGet gRPC services, which relied on
      a single schema and encouraged tightly coupled code. This refactor paves
      the way for implementing server-side streaming support for batch get
      requests more easily.
  • Simplified the request processing logic by replacing the chain of
    responsibility pattern with a single processor class.

How was this PR tested?

WIP (UTs and E2E will be added)

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to explain your proposed changes and call out the behavior change.

@sushantmane sushantmane changed the title [server] Rewrite gRPC read service to bridge gap with Netty-based HTTP server [server] Rewrite gRPC read Service to align with Netty-Based HTTP Server Sep 9, 2024
Copy link
Contributor

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot Sushant! The gRPC code needs a lot of polishing and this PR looks like a very good change. I left a bunch of comments, though they are likely all minor. We also need to fix the static analysis and code coverage... Thanks again, I'm looking forward to seeing this merged in, great work!

@sushantmane sushantmane force-pushed the li-rewrite-grpc-service branch 2 times, most recently from 63de457 to 1f17f5b Compare September 11, 2024 21:08
@sushantmane sushantmane force-pushed the li-rewrite-grpc-service branch 3 times, most recently from fdad603 to 3458eb8 Compare September 13, 2024 12:24
… - Added

missing APIs to the gRPC service to close feature gaps with the HTTP
implementation.  - Fixed the gRPC I/O request path threading model: previously,
requests were handled by the gRPC executor thread pool, but now they are
processed by the `StorageReadExecutor` thread pool for better performance.  -
Deprecated the old `get` and `multiGet` gRPC services, which relied on a single
schema and encouraged tightly coupled code. This refactor paves the way for
implementing server-side streaming support for batch get requests more easily.
- Simplified the request processing logic by replacing the chain of
responsibility pattern with a single processor class.
Copy link
Contributor

@FelixGV FelixGV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing another pass on this PR... thanks for iterating on it. Looks good so far, but I'm not done going through it... I left some minor comments, and I will resume my second pass later...

Comment on lines 7 to +16
public enum ServerAdminAction {
DUMP_INGESTION_STATE(0), DUMP_SERVER_CONFIGS(1);

private static final Map<Integer, ServerAdminAction> ADMIN_ACTION_MAP = new HashMap<>(2);

static {
for (ServerAdminAction action: values()) {
ADMIN_ACTION_MAP.put(action.getValue(), action);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, for the common case of enum codes that range from 0..N monotonically, we have a cleaner solution than using a map... See EnumUtils::getEnumValuesArray and VeniceEnumValue. It is very easy to use, more performant, and will require a bit fewer lines of code than you have here...

Comment on lines -7 to +19
rpc get (VeniceClientRequest) returns (VeniceServerResponse) {}
rpc batchGet(VeniceClientRequest) returns (VeniceServerResponse) {}

rpc get (VeniceClientRequest) returns (VeniceServerResponse) {
option deprecated = true;
}

rpc batchGet(VeniceClientRequest) returns (VeniceServerResponse) {
option deprecated = true;
}

rpc singleGet(SingleGetRequest) returns (SingleGetResponse) {}

rpc multiGet(MultiGetRequest) returns (MultiKeyResponse) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion here... I know I am the one that asked for marking one of the APIs as deprecated, but I did not have the full understanding at the time I said that...

Since we are not yet implementing the new protocol buffers on the client-side, then effectively these old get/batchGet protocols are still the only valid one... the new ones are still experimental, they're not implemented end-to-end, and they might still change before becoming the new protocols we use (e.g. in case we decide to model the multi-key content as separate units).

So given all of the above, I think it's incorrect to mark the old protocols as deprecated, both here and in the Java code which use them.

Sorry again for the flip flopping...

@sushantmane
Copy link
Contributor Author

sushantmane commented Oct 17, 2024

I'm still working on client side changes to support streaming within a single route. I've figured out structure for it and working on changes

@FelixGV
Copy link
Contributor

FelixGV commented Oct 17, 2024

I'm still working on client side changes to support streaming within a single route. I've figured out structure for it and working changes

Nice, looking forward to it. When you're ready with your next iteration, please rebase, push it up and LMK. I'll take another look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants