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

slack-15.0: forward ports slack specific v14 PRS fix #391

Closed
wants to merge 5 commits into from

Conversation

vmogilev
Copy link

@vmogilev vmogilev commented Jun 1, 2024

Description

forward ports slack specific v14 PRS fix: #177

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

Deployment Notes

@vmogilev vmogilev requested a review from a team as a code owner June 1, 2024 01:03
@github-actions github-actions bot added this to the v15.0.5 milestone Jun 1, 2024
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@timvaillancourt timvaillancourt changed the title forward ports slack specific v14 PRS fix slack-15.0: forward ports slack specific v14 PRS fix Jun 4, 2024
// authority holding a shard lock (PRS on vtctld)
log.Infof("slack-debug: we are actively setting replication source, exiting")

return nil
Copy link
Member

Choose a reason for hiding this comment

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

@vmogilev return nil makes the client unaware the repairReplication never happened. Should we return an error to the RPC caller that explains what happened?

@timvaillancourt
Copy link
Member

@vmogilev _isSetReplicationSourceLockedRunning is a concurrently-access struct field, so it needs to be read/updated under a mutex or this causes a race. This field is now protected by the mutex designed for read/writing _ vars in the struct

I attempted to add a unit test so the race detector would prove this to be true, but it's a complicated test to write, so I have deferred this

@timvaillancourt
Copy link
Member

Adding more reviewers as I've now changed code

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
@vmogilev vmogilev closed this Jun 6, 2024
@vmogilev vmogilev deleted the vm_slack-15.0 branch June 6, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants