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

Record the endpoint id used when invoking a service, and restore it when replaying. #573

Merged

Conversation

slinkydeveloper
Copy link
Contributor

Based on #570, Fix #503

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. The changes look good to me. +1 for merging after resolving my minor comments.

}

pub(super) fn chosen_endpoint_to_notify(&mut self) -> Option<EndpointId> {
debug_assert!(matches!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

src/worker/src/partition/effects/interpreter.rs Outdated Show resolved Hide resolved
src/worker/src/partition/effects/interpreter.rs Outdated Show resolved Hide resolved
} => {
metadata.journal_metadata.endpoint_id = Some(endpoint_id);
state_storage
.store_invocation_status(&service_id, InvocationStatus::Invoked(metadata))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment why InvocationStatus::Invoked can be assumed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, check now

@slinkydeveloper slinkydeveloper force-pushed the issues/503_second_attempt branch from 9a95b6a to 1b52615 Compare July 10, 2023 10:02
@slinkydeveloper slinkydeveloper force-pushed the issues/503_second_attempt branch from 055053a to 70188f3 Compare July 10, 2023 12:49
@slinkydeveloper
Copy link
Contributor Author

Tested with restatedev/e2e#167 and it works 🤩

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 updating this PR @slinkydeveloper. LGTM. +1 for merging after resolving my only comment.

@slinkydeveloper slinkydeveloper merged commit 99eed5a into restatedev:main Jul 11, 2023
@slinkydeveloper slinkydeveloper deleted the issues/503_second_attempt branch July 11, 2023 10:11
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.

Record service version with which journal was started
2 participants