Replace current rpc with grpc #691
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
henrysun007
reviewed
Jun 1, 2023
cmake/scripts/test.sh
Outdated
| @@ -152,7 +152,13 @@ run_functional_tests() { | |||
|
|
|||
| ./teaclave_functional_tests -t end_to_end | |||
|
|
|||
| # Run script tests | |||
| python3 -m grpc_tools.protoc \ | |||
There was a problem hiding this comment.
Can we abstract these commands into a function? It seems to appear in many places.
| @@ -58,10 +58,12 @@ simple_asn1 = { git = "https://github.com/acw/simple_asn1", rev = "7db7a48 | |||
| gbdt = { git = "https://github.com/apache/incubator-teaclave-crates" } | |||
| getrandom = { git = "https://github.com/apache/incubator-teaclave-crates" } | |||
| image = { git = "https://github.com/apache/incubator-teaclave-crates" } | |||
| mio = { git = "https://github.com/GeminiCarrie/incubator-teaclave-crates", branch = "rustls_0.19.1"} | |||
There was a problem hiding this comment.
Please open a PR in https://github.com/apache/incubator-teaclave-crates.
rpc/README.md
Outdated
| first. Then, create a client of management service with the channel. At last, | ||
| you can use this client to send requests like `InvokeTask`. | ||
| A channel in gRPC represent a connection to the target service. Clients can use | ||
| the channel to send requests. When constructing a client, you can use the `SgxTrustedTlsClientConfig` to setup TLS and attestation configs, so that we can establish and attested a remote connection. For example, to connect the management service, you need to establish a trusted channel with the service first. Then, create a client of management service with the channel. At last, you can use this client to send requests like `InvokeTask`. |
rpc/README.md
Outdated
|
|
||
| Similar with other RPC frameworks, there are several concepts of RPC in | ||
| Teaclave. | ||
| Re-export `tonic` to support general gRPC framework. `tonic` is a gRPC over HTTP/2 implementation focused on high performance, interoperability, and flexibility. |
There was a problem hiding this comment.
A link to tonic website can be given here.
rpc/README.md
Outdated
| messages, and forwarding requests to certain service. Similar with channel in | ||
| Teaclave, we implement `SgxTrustedTlsServer`, which can establish an attested TLS | ||
| channel with clients. | ||
| Server is an entity to listening a network address, processing incoming messages, and forwarding requests to certain service. Similar with the client, you can use `SgxTrustedTlsServerConfig` to setup TLS and attestation configs for chanenel with clients. |
There was a problem hiding this comment.
Please seperate this one line to different ones as the left does for easy reading.
| TeaclaveServiceResponseError::RequestError(error.to_string()) | ||
| let msg = error.to_string(); | ||
| let code = match error { | ||
| SchedulerServiceError::StorageError | SchedulerServiceError::TaskQueueEmpty => { |
There was a problem hiding this comment.
It seems TaskQueueEmpty is not neccary to be data loss.
| TeaclaveServiceResponseError::RequestError(error.to_string()) | ||
| let msg = error.to_string(); | ||
| let code = match error { | ||
| StorageServiceError::Database(_) => Code::DataLoss, |
There was a problem hiding this comment.
Is database error necessarily be a data loss?
| { | ||
| *ncases += 1; | ||
|
|
||
| let t = || -> f64 { |
There was a problem hiding this comment.
please resue the code of function test.
| let response = self.get(r)?; | ||
| Ok(response).map(TeaclaveStorageResponse::Get) | ||
| } | ||
| TeaclaveStorageRequest::Put(r) => { |
There was a problem hiding this comment.
It seems the code can be simplified by combinding different lines.
| } | ||
|
|
||
| #[teaclave_rpc::async_trait] | ||
| impl TeaclaveStorage for ProxyService { |
There was a problem hiding this comment.
The first inputs are all self. How about using an associated function here?
GeminiCarrie
force-pushed
the
grpc_new
branch
3 times, most recently
from
154ef58
to
8beec45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
This PR replaces current rpc with grpc.
issues
Type of change (select or add applied and delete the others)
How has this been tested?
Checklist
master.