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

Add service ready signal #218

Merged
merged 6 commits into from
Aug 3, 2022
Merged

Add service ready signal #218

merged 6 commits into from
Aug 3, 2022

Conversation

cafca
Copy link
Member

@cafca cafca commented Aug 1, 2022

Services receive an additional parameter with a oneshot channel to which they send a unit value once they are ready to process messages on the communication bus. This allows us to understand when a service becomes ready from outside the service. Previously there were a couple of tests that waited for some arbitrary time to allow services to become available.

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #218 (39f9a7c) into main (bbc55d5) will increase coverage by 0.02%.
The diff coverage is 89.20%.

@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
+ Coverage   93.70%   93.73%   +0.02%     
==========================================
  Files          42       44       +2     
  Lines        3179     3496     +317     
==========================================
+ Hits         2979     3277     +298     
- Misses        200      219      +19     
Impacted Files Coverage Δ
aquadoggo/src/db/stores/test_utils.rs 98.12% <ø> (ø)
aquadoggo/src/node.rs 0.00% <ø> (ø)
aquadoggo/src/materializer/worker.rs 86.04% <44.82%> (-4.41%) ⬇️
aquadoggo/src/http/service.rs 91.42% <75.00%> (-2.33%) ⬇️
aquadoggo/src/replication/service.rs 81.87% <76.92%> (-0.86%) ⬇️
aquadoggo/src/materializer/service.rs 94.92% <86.48%> (-2.19%) ⬇️
aquadoggo/src/materializer/tasks/schema.rs 87.95% <87.95%> (+87.95%) ⬆️
aquadoggo/src/materializer/tasks/reduce.rs 95.55% <88.88%> (+5.67%) ⬆️
aquadoggo/src/materializer/tasks/dependency.rs 98.03% <95.83%> (+3.56%) ⬆️
aquadoggo/src/db/provider.rs 95.34% <97.14%> (+7.84%) ⬆️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cafca cafca marked this pull request as ready for review August 1, 2022 17:30
Copy link
Member

@adzialocha adzialocha left a comment

Choose a reason for hiding this comment

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

Just requested some small changes, otherwise this look super 👍

context: Context,
signal: Shutdown,
tx: ServiceSender,
tx_ready: oneshot::Sender<()>,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe give the sender also an own type like you already do for the receiver (ServiceReady)? I think that would also ready easier as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Like this?

aquadoggo/src/manager.rs Outdated Show resolved Hide resolved
This was referenced Aug 3, 2022
@cafca cafca requested a review from adzialocha August 3, 2022 16:06
@adzialocha adzialocha merged commit dd73fe5 into main Aug 3, 2022
@adzialocha adzialocha deleted the service-ready-signal branch August 19, 2022 15:20
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.

3 participants