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

Adds an RWLock to protect replication from truncation. #119

Closed
wants to merge 1 commit into from

Conversation

slackpad
Copy link
Contributor

This is a very simple fix for the "replace single row lookups with multi row lookups" part of #84. I looked at extending the LogStore interface but it was awkward to do and added a lot of complexity, when a simple lock at this higher level adds protection without pushing any of those details downwards.

I'm thinking through a good way to test this, other than the current tests which are passing as a regression.

@slackpad
Copy link
Contributor Author

slackpad commented Apr 19, 2016

@ongardie or @superfell I wanted to clarify the intent of the "verify the current term has not changed when preparing the AppendEntries message" companion item to this one from #84. It doesn't seem like we will spend much time preparing the AppendEntries message and the upper code that calls this will handle the leader stepping down on the next replication attempt. Was there a particular edge you were trying to catch with the additional check?

@armon armon mentioned this pull request Apr 20, 2016
14 tasks
@ongardie
Copy link
Contributor

@slackpad I think the main concerns were in the following sequences:

  1. set AppendEntries previous log index + term
  2. leader steps down
  3. log changes
  4. set AppendEntries entries

or

  1. set AppendEntries previous log index + term
  2. set AppendEntries entries[:x]
  3. leader steps down
  4. log changes
  5. set AppendEntries entries[x:]

@ongardie-sfdc
Copy link

Looking at this again, I don't think we need or want the logsLock. I think re-checking the server's term will buy us more safety for the scenarios I mentioned in my last comment. So 👎 on this one. Let me give it a try in a later PR.

@slackpad slackpad closed this Jul 9, 2016
@slackpad
Copy link
Contributor Author

slackpad commented Jul 9, 2016

Agreed - this isn't the right approach.

@slackpad slackpad deleted the f-logs-lock branch July 9, 2016 03:38
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.

3 participants