Skip to content

Keep alive apps and scheduled-stop #2230

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
Merged
merged 7 commits into from
Jun 9, 2020
Merged

Keep alive apps and scheduled-stop #2230

merged 7 commits into from
Jun 9, 2020

Conversation

coot
Copy link
Contributor

@coot coot commented Jun 9, 2020

No description provided.

@coot coot requested a review from karknu June 9, 2020 05:45
karknu
karknu requested changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

The functionality doesn't match what we need for a keep-alive miniprotocol.
Without a req-rsp model it is not possible to get the needed RTT measurement and it is not possible for the client to learn that the server is down in a timely manner.

@coot
Copy link
Contributor Author

coot commented Jun 9, 2020

@karknu this is req-resp model. Every KeepAliveInterval the client sends MsgKeepAlive and the server replies with MsgKeepAliveResponse as soon as it receives client message.

coot added 3 commits June 9, 2020 08:41
@coot
KeepAlive: limits
e97f384 
- byteLimitsKeepAlive: with a test
- timeLimitsKeepAlive:
    10s limit for waiting for server's reply,
    no limit on a client's message.
@coot
cardano-client: include STM m RunOrStop
0b1d100 
This might not be needed at all for clients.  To provide it we'd need
a distinction between client and server `OuroborosApplication`.  I think
it's better to do this at a later stage.
@coot
cardano-client: clean usage of IO and m
6e8cce9 
subscribe must use `IO`, because of `ncSubscriptionWorker`, but
`versionedProtocols` can be polymorphic in its runtime monad.
@coot coot force-pushed the coot/keep-alive-applications branch from f5f8ec8 to 6e8cce9 Compare June 9, 2020 07:08
@coot coot requested a review from karknu June 9, 2020 07:09
dm1mf
dm1mf approved these changes Jun 9, 2020
View reviewed changes
Copy link

@dm1mf dm1mf left a comment

Choose a reason for hiding this comment

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

Looks good to me.

MarcFontaine
MarcFontaine approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@MarcFontaine MarcFontaine left a comment

Choose a reason for hiding this comment

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

LGTM,
(I only looked at the changes in Cardano.Client.Subscription)

@karknu
Copy link
Contributor

karknu commented Jun 9, 2020

@karknu this is req-resp model. Every KeepAliveInterval the client sends MsgKeepAlive and the server replies with MsgKeepAliveResponse as soon as it receives client message.

You're right.

karknu
karknu approved these changes Jun 9, 2020
View reviewed changes
Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM.

@coot
Copy link
Contributor Author

coot commented Jun 9, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jun 9, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit fae4877 into master Jun 9, 2020
@iohk-bors iohk-bors bot deleted the coot/keep-alive-applications branch June 9, 2020 14:15
coot added a commit that referenced this pull request May 16, 2022
@iohk-bors @coot
Merge #2230
c24b679 
2230: Keep alive apps and scheduled-stop r=coot a=coot



Co-authored-by: Marcin Szamotulski <profunctor@pm.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@karknu karknu karknu approved these changes

+2 more reviewers

@MarcFontaine MarcFontaine MarcFontaine approved these changes

@dm1mf dm1mf dm1mf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@coot @karknu @MarcFontaine @dm1mf