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

Ensure flags, switches, and samples are read from write DB #249

Merged

Conversation

dtao
Copy link
Contributor

@dtao dtao commented Jul 21, 2017

In an environment with sufficiently high traffic and DB replication set
up, there is a race condition with switches and samples where when an
update is immediately followed by a read, the read will get a stale
value from the DB replica and cache it.

This fixes the race condition by ensuring that switches and samples are
always read from the write DB.

@jsocol
Copy link
Collaborator

jsocol commented Jul 21, 2017

Ah, yes that's definitely a concern, but I don't know if the right thing to do is throw all that read traffic at the write DB—at least without letting users opt out of that. What about write-through cache updates?

@dtao
Copy link
Contributor Author

dtao commented Jul 21, 2017

I'm definitely open to taking that approach. It makes me a little nervous (but only a little) because there's technically still a race condition in that case where the DB and cache could get out of sync if there are two near-simultaneous writes.

I will also point out that "all that read traffic" is kept to an absolute minimum by waffle's design. Because of the way waffle is implemented, it almost never hits the DB for switches or samples (depending on cache expiration of course).

That said if your preference in light of these considerations is to take the write-through approach just say the word and I'll be happy to create a different PR that goes that route.

@dtao
Copy link
Contributor Author

dtao commented Jul 21, 2017

Oh and I am also happy to keep this as is but add support for a Django setting (maybe something like READ_SWITCHES_FROM_WRITE_DB) to make this behavior configurable.

@jsocol
Copy link
Collaborator

jsocol commented Jul 21, 2017

True, you make good points. Let's put it behind a setting (defaulting to the current behavior) and add a note to the docs?

@dtao
Copy link
Contributor Author

dtao commented Jul 22, 2017

OK, I've updated the PR to make the behavior configurable (disabled by default) via the WAFFLE_READ_FROM_WRITE_DB setting, which I've added to the documentation as well.

Note that I also extended this setting to flags along with switches and samples, as after taking a closer look at how flags work I realized this makes just as much sense for flags.

Let me know if you need me to squash this into a single commit before it can be merged.

In an environment with sufficiently high traffic and DB replication set
up, there is a race condition where when an update is immediately followed by
a read, the read will get a stale value from the DB replica and cache it.

This fixes the race condition by ensuring that flags, switches, and samples
are always read from the write DB.
@dtao dtao force-pushed the read-switches-and-samples-from-write-db branch from 9bdb3e1 to f4d6243 Compare July 28, 2017 19:59
@dtao dtao changed the title Ensures switches & samples are read from write DB Ensure flags, switches, and samples are read from write DB Jul 28, 2017
@dtao
Copy link
Contributor Author

dtao commented Jul 28, 2017

Update: I went ahead and squashed the commits in this PR, since that's what it says to do in CONTRIBUTING.rst.

@stale stale bot added the stale label Aug 27, 2017
@stale
Copy link

stale bot commented Aug 27, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Sep 10, 2017
@stale
Copy link

stale bot commented Sep 10, 2017

This issue has been closed as stale because it has not had any recent activity. Please feel free to re-open if the issue is still relevant. Thank you for your contributions!

@dtao
Copy link
Contributor Author

dtao commented Sep 11, 2017

Is it possible to re-open this? I definitely understand having an automated process for closing stale PRs; but unless I'm missing something this PR is basically good to merge and was just waiting on a maintainer to make sure it looked good and click the button.

@jsocol
Copy link
Collaborator

jsocol commented Sep 11, 2017

Yes, re-opening. Honestly I didn't expect the bot to apply to PRs, we'll probably need to tweak it a bit, and I'm going to go back through the PRs it closed and re-open still-relevant ones.

@jsocol jsocol reopened this Sep 11, 2017
@stale stale bot removed the stale label Sep 11, 2017
@stale
Copy link

stale bot commented Oct 11, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 11, 2017
@dtao
Copy link
Contributor Author

dtao commented Oct 11, 2017

Oh, stale bot...

@stale stale bot removed the stale label Oct 11, 2017
@jsocol
Copy link
Collaborator

jsocol commented Oct 11, 2017

I need to tweak those settings, it's using the defaults and that's too aggressive.

@stale
Copy link

stale bot commented Nov 10, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 10, 2017
@dtao
Copy link
Contributor Author

dtao commented Nov 10, 2017

One of these days ;)

@stale stale bot removed the stale label Nov 10, 2017
@ojongerius
Copy link

Are there any actionable items before this can go in?

Copy link
Collaborator

@jsocol jsocol left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Merging!

@jsocol jsocol merged commit 8486108 into jazzband:master Nov 21, 2017
@dtao
Copy link
Contributor Author

dtao commented Nov 22, 2017

Sweet! Out of curiosity, what's the process for releasing new versions? Is that entirely up to the discretion of @jsocol or should I be doing something (like creating a version bump PR)?

No rush, just making sure I'm not dropping the ball if it's supposed to be up to me at this point.

@jsocol
Copy link
Collaborator

jsocol commented Nov 22, 2017

Me or any of the other maintainers. I’ll probably have time to take a look at it later during this holiday weekend.

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

3 participants