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

Introduce interface to export InvocationStatus #526

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

slinkydeveloper
Copy link
Contributor

Fix #517

@slinkydeveloper slinkydeveloper added this to the 1B milestone Jun 23, 2023
@slinkydeveloper
Copy link
Contributor Author

cc @igalshilman

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR @slinkydeveloper. I had a few questions about the guarantees of the Weak pointer and what the user of InvokerStatusReader should see.

src/invoker/src/invocation_task.rs Outdated Show resolved Hide resolved
src/invoker/src/invocation_task.rs Outdated Show resolved Hide resolved
src/invoker/src/invoker.rs Outdated Show resolved Hide resolved
src/invoker/src/invoker.rs Show resolved Hide resolved
src/invoker/src/invoker.rs Outdated Show resolved Hide resolved
src/invoker/src/lib.rs Outdated Show resolved Hide resolved
src/invoker/src/lib.rs Outdated Show resolved Hide resolved
src/invoker/src/status.rs Show resolved Hide resolved
// -- Status data structure

#[derive(Debug)]
pub struct InvocationStatusReport(
Copy link
Contributor

Choose a reason for hiding this comment

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

@slinkydeveloper The API looks exactly what we would need at the query side!

We might add/remove fields over time to fine tune the table structure that we'll expose.

One question tho, to you and @tillrohrmann - what do you think about moving this to common, so that we don't add an explicit dependency to the invoker?
Or alternatively, if possible create a thin api crate for the invoker - wdyt?

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Jun 26, 2023

Choose a reason for hiding this comment

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

Or alternatively, if possible create a thin api crate for the invoker - wdyt?

Probably we'll do the latter. See #392 and #420 and #528

Copy link
Contributor

@igalshilman igalshilman Jun 26, 2023

Choose a reason for hiding this comment

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

Ok then, I will wait until this will be moved to the common crate or an API crate.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

LGTM. +1 for merging.

@slinkydeveloper slinkydeveloper merged commit 001aca9 into restatedev:main Jun 26, 2023
5 checks passed
@slinkydeveloper slinkydeveloper deleted the issues/517 branch June 26, 2023 09:36
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.

Define an interface to read and update invoker's InvocationState
3 participants