Skip to content

Commit

Permalink
[Sharded-Execution-GRPC] Address some review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
manudhundi committed Sep 29, 2023
1 parent c962909 commit c453975
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 4 deletions.
6 changes: 6 additions & 0 deletions execution/executor-service/src/remote_state_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ impl RemoteStateValueReceiver {
.inner
.into_iter()
.for_each(|(state_key, state_value)| {
// Though we try to insert the state key during the 'pre_fetch_state_values' remote
// request, there is a possibility that the state key is not inserted before we
// receive and process the response here. To be on the safe side, we insert the
// state key here if it is not inserted already. This does not matter because we
// do not have versioning of the state view.
state_view_lock.insert_state_key(state_key.clone());
state_view_lock.set_state_value(&state_key, state_value);
});
}
Expand Down
1 change: 0 additions & 1 deletion execution/executor-service/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ pub fn create_thread_remote_executor_shards(
}

#[test]
#[ignore]
fn test_sharded_block_executor_no_conflict() {
use std::thread;

Expand Down
9 changes: 6 additions & 3 deletions secure/net/src/grpc_network_service/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,12 @@ impl GRPCNetworkMessageServiceServerWrapper {
.unwrap();

info!("Starting Server async at {:?}", server_addr);
// NOTE: serve_with_shutdown() starts the server, if successful the task does not return
// till the server is shutdown. Hence this should be called as a separate non-blocking task.
// Signal handler 'server_shutdown_rx' is needed to shutdown the server
// NOTE: (1) serve_with_shutdown() starts the server, if successful the task does not return
// till the server is shutdown. Hence this should be called as a separate
// non-blocking task. Signal handler 'server_shutdown_rx' is needed to shutdown
// the server
// (2) There is no easy way to know if/when the server has started successfully. Hence
// we may need to implement a healthcheck service to check if the server is up
Server::builder()
.timeout(std::time::Duration::from_millis(rpc_timeout_ms))
.add_service(NetworkMessageServiceServer::new(self))
Expand Down
3 changes: 3 additions & 0 deletions secure/net/src/network_controller/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ impl NetworkController {
self.outbound_task_shutdown_tx = self.outbound_handler.start(&self.outbound_rpc_runtime);
}

// TODO: This is still not a very clean shutdown. We don't wait for the full shutdown after
// sending the signal. May not matter much for now because we shutdown before exiting the
// process. Ideally, we want to fix this.
pub fn shutdown(&mut self) {
info!("Shutting down network controller at {}", self.listen_addr);
if let Some(shutdown_signal) = self.inbound_server_shutdown_tx.take() {
Expand Down

0 comments on commit c453975

Please sign in to comment.