Skip to content

[ReplicatedLoglet] Implement remote sequencer find tail #2017

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

Closed
wants to merge 2 commits into from

Conversation

muhamadazmy
Copy link
Contributor

@muhamadazmy muhamadazmy commented Oct 2, 2024

[ReplicatedLoglet] Implement remote sequencer find tail


Stack created with Sapling. Best reviewed with ReviewStack.

Summary:
Implements a remote loglet append calls to leader sequencer
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 @muhamadazmy. What happens if we lose the GetSequencerInfo or its response?

Comment on lines +232 to +243
.call(
&self.networking,
self.params.sequencer,
GetSequencerInfo {
header: CommonRequestHeader {
log_id: self.log_id,
loglet_id: self.params.loglet_id,
segment_index: self.segment_index,
},
},
)
.await
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if the request or response get lost? Would sync_sequencer_tail get stuck?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's definitely something that need to be handled. I have paused working on this draft PR in favour of #2019 and wasn't sure if I should continue on this one.

);
}
_ => {
unreachable!()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe state why this case is unreachable. Maybe also don't use the wildcard. That way we'll see the places where a newly SequencerStatus variant needs to be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Thank you :)

Comment on lines +277 to +279
async fn sync_log_servers_tail(&self) -> Result<(), OperationError> {
todo!()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's where #2019 comes into play, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I started on this before #2019 was available. I think #2019 renders this PR obsolete since fetching the tail from the sequencer is just an improvement.

Comment on lines +218 to +224
if self.sync_sequencer_tail().await.is_ok() {
return Ok(*self.known_global_tail.get());
}

// otherwise we need to try to fetch this from the log servers.
self.sync_log_servers_tail().await?;
Ok(*self.known_global_tail.get())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there value in racing these two variants?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I get your question here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whether we should run both variants for obtaining the known global tail in parallel/concurrently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for clarification. Yeah, that's definitely a good idea.

@muhamadazmy muhamadazmy closed this Oct 7, 2024
@muhamadazmy muhamadazmy deleted the pr2017 branch October 7, 2024 07:09
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.

2 participants