[ENH] clean up component channels #2429

codetheweb · 2024-06-28T21:01:00Z

This is a small refactor to clean up internal types around component channels. Changes include:

renames for clarity
split up receiver and message wrapper related code into separate files
makes the Sender ("ComponentSender") an internal concept not exposed to components directly
makes the ComponentHandle threadsafe so it can be passed around and .clone()d

Should make the implementation of #2428 cleaner.

github-actions · 2024-06-28T21:01:12Z

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

Can you think of any use case in which the code does not behave as intended? Have they been tested?
Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
If appropriate, are there adequate property based tests?
If appropriate, are there adequate unit tests?
Should any logging, debugging, tracing information be added or removed?
Are error messages user-friendly?
Have all documentation changes needed been made?
Have all non-obvious changes been commented?

System Compatibility

Are there any potential impacts on other parts of the system or backward compatibility?
Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

codetheweb · 2024-06-28T22:35:33Z

rust/worker/src/server.rs

@@ -116,7 +117,7 @@ impl WorkerServer {
 Ok(())
 }

- pub(crate) fn set_dispatcher(&mut self, dispatcher: Box<dyn Receiver<TaskMessage>>) {
+ pub(crate) fn set_dispatcher(&mut self, dispatcher: ComponentHandle<Dispatcher>) {


I'm fine keeping this as Box<dyn Receiver<TaskMessage>> if that style is preferred

i prefer this! nice cleanup

codetheweb · 2024-06-28T23:13:55Z

rust/worker/src/system/types.rs

+ self.sender
+ .send(wrap(message, tracing_context))
+ .await
+ .map_err(|_| ChannelError::SendError)


was hoping to get rid of this struct and make it a type alias, but there's 3 separate places that would have to implement the same wrapping code here in that case

(and while these 4 lines are straightforward, the upcoming .request() method will be more complex and should really only be defined once)

rust/worker/Dockerfile

HammadB · 2024-07-01T17:57:55Z

rust/worker/src/compactor/compaction_manager.rs

@@ -42,7 +43,7 @@ pub(crate) struct CompactionManager {
 blockfile_provider: BlockfileProvider,
 hnsw_index_provider: HnswIndexProvider,
 // Dispatcher
- dispatcher: Option<Box<dyn Receiver<TaskMessage>>>,
+ dispatcher: Option<ComponentHandle<Dispatcher>>,


this looks so much nicer

HammadB · 2024-07-01T18:49:37Z

rust/worker/src/compactor/compaction_manager.rs

@@ -95,7 +96,7 @@ impl CompactionManager {
 compaction_job: &CompactionJob,
 ) -> Result<CompactionResponse, Box<dyn ChromaError>> {
 let dispatcher = match self.dispatcher {
- Some(ref dispatcher) => dispatcher,
+ Some(ref dispatcher) => dispatcher.clone(),


do we need to clone here?

yes, because CompactOrchestrator takes ownership

rust/worker/src/execution/dispatcher.rs

rust/worker/src/system/types.rs

HammadB

Nice - have some ignorable / disagreeable nits but I think the locking changes we should make and ideally stop() doesn't have to be async then

[ENH] [WIP] clean up component channels

b559fb7

codetheweb force-pushed the chore-cleanup-component-channels branch from e4f3450 to ba9fd7f Compare June 28, 2024 21:53

Cleanup

3c7d4c8

codetheweb force-pushed the chore-cleanup-component-channels branch from ba9fd7f to 3c7d4c8 Compare June 28, 2024 21:59

codetheweb added 2 commits June 28, 2024 15:16

Rename

6b652f6

Renames

94efff4

codetheweb commented Jun 28, 2024

View reviewed changes

codetheweb added 2 commits June 28, 2024 15:44

Receiver impl wrapper unnecessary

ef4a621

Share impl

e3ab429

codetheweb force-pushed the chore-cleanup-component-channels branch from a398448 to e3ab429 Compare June 28, 2024 23:00

Add comments

f66422c

codetheweb commented Jun 28, 2024

View reviewed changes

Bump Rust version in Dockerfile

d010655

codetheweb commented Jun 28, 2024

View reviewed changes

rust/worker/Dockerfile Show resolved Hide resolved

codetheweb changed the title ~~[ENH] [WIP] clean up component channels~~ [ENH] clean up component channels Jun 28, 2024

codetheweb marked this pull request as ready for review June 28, 2024 23:29

codetheweb requested a review from HammadB June 28, 2024 23:29

Fix test

5b45330