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.

Sign up for GitHub

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

Jump to bottom

New start strategy for miniprotocols #5048

Merged
merged 4 commits into from
Jan 29, 2025
Merged

New start strategy for miniprotocols #5048

merged 4 commits into from
Jan 29, 2025

Conversation

karknu
Copy link
Contributor

@karknu karknu commented Jan 22, 2025

Description

Add a new start strategy for miniprotocols. StartOnDemandAny will start the miniprotocol as soon as any other StartOnDemand protocols start.
StartOnDemandAny is used by the responder side for KeepAlive to ensure that it is always active when any other protocol is running.

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

@karknu
Copy link
Contributor Author

karknu commented Jan 22, 2025

Fixes #5037

@karknu karknu mentioned this pull request Jan 22, 2025
Closed
7 tasks
@karknu karknu force-pushed the karknu/startondemandany branch from f7bf7c8 to 1d3b911 Compare January 23, 2025 09:39
@coot coot mentioned this pull request Jan 27, 2025
Open
@karknu karknu marked this pull request as ready for review January 28, 2025 09:59
@karknu karknu requested a review from a team as a code owner January 28, 2025 09:59
@karknu karknu force-pushed the karknu/startondemandany branch 3 times, most recently from 6ec7cda to d0a5ebc Compare January 28, 2025 10:07
@coot coot added the mux issues related to network-mux label Jan 28, 2025
crocodile-dentist
crocodile-dentist approved these changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@crocodile-dentist crocodile-dentist left a comment

Choose a reason for hiding this comment

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

lgtm, only one nomenclature comment.

network-mux/src/Network/Mux.hs Show resolved Hide resolved
coot
coot requested changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

Nice, thanks Karl; a few remarks.

network-mux/src/Network/Mux.hs Outdated Show resolved Hide resolved
network-mux/src/Network/Mux.hs Outdated Show resolved Hide resolved
network-mux/src/Network/Mux.hs Outdated Show resolved Hide resolved
network-mux/src/Network/Mux/Types.hs Outdated Show resolved Hide resolved
network-mux/test/Test/Mux.hs Outdated Show resolved Hide resolved
network-mux/test/Test/Mux.hs Show resolved Hide resolved
ouroboros-network/src/Ouroboros/Network/Diffusion/NonP2P.hs Outdated Show resolved Hide resolved
@karknu karknu force-pushed the karknu/startondemandany branch from cb4b76d to aa7bc10 Compare January 29, 2025 10:22
coot
coot reviewed Jan 29, 2025
View reviewed changes
Copy link
Contributor

@coot coot left a comment

Choose a reason for hiding this comment

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

LGTM

network-mux/src/Network/Mux.hs Outdated Show resolved Hide resolved
@karknu karknu force-pushed the karknu/startondemandany branch from 781e774 to 8b3ed1e Compare January 29, 2025 12:27
karknu added 4 commits January 29, 2025 13:40
@karknu
New start strategy for miniprotocols
8554e74 
Add a new start strategy for miniprotocols. StartOnDemandAny will
start the miniprotocol as soon as any other StartOnDemand protocols
start.
StartOnDemandAny is used by the responder side for KeepAlive to ensure
that it is always active when any other protocol is running.
@karknu
Testing of StartOnDemandAny
4eceb5a 
Adapt test cases so that the StartOnDemandAny strategy is tested too.
@karknu
Use the correct trace for StartOnDemand
1470848 
TraceStartOnDemand should be used when we intend to start a miniprotocol
on demand. TraceStartedOnDemand should be used once the miniprotocol has
started.
@karknu
Update CHANGELOGs.
06beeb2
@karknu karknu force-pushed the karknu/startondemandany branch from 8b3ed1e to 06beeb2 Compare January 29, 2025 12:40
@karknu karknu enabled auto-merge January 29, 2025 12:41
@karknu karknu added this pull request to the merge queue Jan 29, 2025
Merged via the queue into main with commit f4d05d1 Jan 29, 2025
12 checks passed
@karknu karknu deleted the karknu/startondemandany branch January 29, 2025 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crocodile-dentist crocodile-dentist crocodile-dentist approved these changes

@coot coot coot approved these changes

Assignees

@karknu karknu

Labels
mux issues related to network-mux
Projects
Ouroboros Network
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karknu @coot @crocodile-dentist