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

feat: Expose endpoint to control streams #430

Merged
merged 17 commits into from
Dec 14, 2023

Conversation

morgsmccauley
Copy link
Collaborator

@morgsmccauley morgsmccauley commented Nov 26, 2023

This PR exposes a gRPC endpoint for the Block Streamer service to provide control over individual Block Streams. There are currently 3 methods: start/stop/list. The functionality across these methods is quite basic, I didn't want to spend too much time fleshing them out as I'm still not certain on how exactly they will be used. I expect them to Change once the Control Plane starts using them.

@morgsmccauley morgsmccauley linked an issue Nov 26, 2023 that may be closed by this pull request
@morgsmccauley morgsmccauley changed the base branch from main to 418-created-dedicated-block-stream-per-indexer November 26, 2023 22:14
@morgsmccauley morgsmccauley force-pushed the 420-expose-endpoint-to-control-streams branch 3 times, most recently from e1ac0e6 to 5511837 Compare December 3, 2023 22:09
@@ -0,0 +1,5 @@
fn main() -> Result<(), Box<dyn std::error::Error>> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Generates the Rust types from the proto file on cargo build

@@ -0,0 +1,17 @@
use tonic::Request;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These examples use the exposed Rust types to interact with the gRPC service - these not only serve as examples, but also make it easy to quickly debug by running them

Comment on lines +18 to +21
// pub code: String,
// pub start_block_height: Option<u64>,
// pub schema: Option<String>,
// pub provisioned: bool,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No longer used/needed - this is more for the Runner side of things which will be passed from the Control Plane directly

pub indexer_rule: IndexerRule,
}

impl IndexerConfig {
pub fn get_full_name(&self) -> String {
format!("{}/{}", self.account_id, self.function_name)
}

pub fn get_hash_id(&self) -> String {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Consistent/deterministic ID to use for the BlockStream so it can be persisted across restarts

tonic::include_proto!("blockstreamer");
}

pub use blockstreamer::*;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exposes the Rust Client types


Ok(Response::new(response))
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should probably add tests here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@morgsmccauley morgsmccauley marked this pull request as ready for review December 3, 2023 22:17
@morgsmccauley morgsmccauley requested a review from a team as a code owner December 3, 2023 22:17
@morgsmccauley morgsmccauley changed the title 420 expose endpoint to control streams feat: Expose endpoint to control streams Dec 10, 2023
Base automatically changed from 418-created-dedicated-block-stream-per-indexer to main December 11, 2023 20:34

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let mut client = BlockStreamerClient::connect("http://[::1]:10000").await?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to create a client for each request? Can we just use one client instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a cargo example, a standalone binary which can be executed via cargo --example list_streams. I created it so we can easily send requests to the server, rather than creating verbose cli commands.

std::cmp::max(last_block_in_index, last_indexed_block)
});

tracing::debug!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. So, here is where we'd end up with duplicated reads since each stream has its own lake stream? I can see why we would need to rethink the caching as you had briefly mentioned in the weekly meeting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep exactly - we'd need to create some shared client so it can cache internally, but that's a future problem :)

@@ -0,0 +1,142 @@
pub use redis::{self, aio::ConnectionManager, FromRedisValue, ToRedisArgs};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like I saw these changes already. I don't know if a rebase is needed or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah sorry @darunrs, this PR was pointing to #428 so included the commits from that branch. I thought the commits would be removed after merging but that only works for merge commits, not squash merge.

I've removed the commits from the base PR.

@morgsmccauley morgsmccauley force-pushed the 420-expose-endpoint-to-control-streams branch from 9322584 to 2d570f0 Compare December 14, 2023 00:11
@morgsmccauley morgsmccauley merged commit 9c5ee15 into main Dec 14, 2023
2 checks passed
@morgsmccauley morgsmccauley deleted the 420-expose-endpoint-to-control-streams branch December 14, 2023 00:13
@morgsmccauley morgsmccauley mentioned this pull request Dec 19, 2023
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.

Expose endpoint to control streams
2 participants