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
merged 1 commit into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@dtao
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jsocol

jsocol Jul 21, 2017

Owner

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?

Owner

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

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Jul 21, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Jul 21, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jsocol

jsocol Jul 21, 2017

Owner

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

Owner

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

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Jul 22, 2017

Contributor

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.

Contributor

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.

ensure flags, switches, and samples are read from write DB
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 changed the title from Ensures switches & samples are read from write DB to Ensure flags, switches, and samples are read from write DB Jul 28, 2017

@dtao

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Jul 28, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot 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 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

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot 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!

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

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Sep 11, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jsocol

jsocol Sep 11, 2017

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot 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 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

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Oct 11, 2017

Contributor

Oh, stale bot...

Contributor

dtao commented Oct 11, 2017

Oh, stale bot...

@stale stale bot removed the stale label Oct 11, 2017

@jsocol

This comment has been minimized.

Show comment
Hide comment
@jsocol

jsocol Oct 11, 2017

Owner

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

Owner

jsocol commented Oct 11, 2017

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

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot 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 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

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Nov 10, 2017

Contributor

One of these days ;)

Contributor

dtao commented Nov 10, 2017

One of these days ;)

@stale stale bot removed the stale label Nov 10, 2017

@ojongerius

This comment has been minimized.

Show comment
Hide comment
@ojongerius

ojongerius Nov 13, 2017

Are there any actionable items before this can go in?

ojongerius commented Nov 13, 2017

Are there any actionable items before this can go in?

@jsocol

jsocol approved these changes Nov 21, 2017

Sorry for the delay. Merging!

@jsocol jsocol merged commit 8486108 into jsocol:master Nov 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dtao

This comment has been minimized.

Show comment
Hide comment
@dtao

dtao Nov 22, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jsocol

jsocol Nov 22, 2017

Owner

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

Owner

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