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

clean up record logic #4

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

clean up record logic #4

wants to merge 1 commit into from

Conversation

kgioioso
Copy link
Collaborator

@kgioioso kgioioso commented Sep 18, 2023

This PR changes the record logic.

Previously, when a duplicate request is received (either because the client retries the request or the replica receives a missed index), the duplicate entry is added to ProcessQueue. The duplicate entry is then handled depending on the status of the entry.

In this PR, if a duplicate request is received, recordTd checks the status of the entry. If PROCESSED, the entry is routed to fastReplyQu so the reply can be resent. If not processed, the other request is still being processed, and recordTd does nothing with the new request.

This significantly simplifies the logic of processTd because now processTd only handles entries with status INITIAL.

@@ -137,7 +137,7 @@ struct LogEntry {
LogEntry* next; // The next LogEntry pointer

std::string result; // The execution result of the LogEntry
char status; //
char status; // TODO(Katie): does this need to be thread safe?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Steamgjk Does this need to be an atomic type? It seems like recordTd could be reading this value while processTd is writing it.

@kgioioso
Copy link
Collaborator Author

@Steamgjk this is the first logic change I have made. I believe this change does not affect correctness, but I may be missing some nuances of the codebase. Can you please double check my logic? Thanks.

@kgioioso kgioioso marked this pull request as ready for review September 19, 2023 00:30
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.

None yet

1 participant