Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Bug 1238696: Update django-mozilla-product-details #3767

Closed
wants to merge 1 commit into from

Conversation

willkg
Copy link
Contributor

@willkg willkg commented Jan 27, 2016

This updates django-mozilla-product-details to 0.10 which was recently
released. This allows us to switch from the file system backend to the
db backend in a future commit.

To test:

  1. wait for travis to run the tests
  2. run "./manage.py update_product_details"

Pretty sure this should be fine. I want to land this before switching
the backend because @jezdez said MDN had problems updating it in the
past and had to revert.

Quick r?

This updates django-mozilla-product-details to 0.10 which was recently
released. This allows us to switch from the file system backend to the
db backend in a future commit.
@jezdez
Copy link
Contributor

jezdez commented Jan 27, 2016

As I said in the ticket, we use product_details at a point of the system where data loading from the database isn't safe (as it's often for import time code parts like model and form fields).

The issues in the past were death spiral usage patterns since product_details was fetching JSON all the time. I would argue this update would need proper load testing on stage.

I'm not sure if using the database for this information is the right place tbh, and frankly I'm not so stoked about the architecture of django-product-details in the first place. I'd rather keep pulling in JSON data during the deploy than having more moving parts for a dataset that rarely changes.

@willkg
Copy link
Contributor Author

willkg commented Jan 27, 2016

@jezdez This PR is literally just updating the version--it doesn't do anything else. I think we should talk about the db backend stuff in the bug.

I'm not sure what you mean by "death spiral usage patterns since product_details was fetching JSON all the time" means. We're only updating the JSON files via cron right now and I think that's once a day or something like that. How would that create load problems?

@jezdez
Copy link
Contributor

jezdez commented Jan 27, 2016

@willkg You're right, this only is a tiny update, but the last time we tried it, the site went down, so please undertsand my caution. We tried to use memcache as its cache backend then and not locmem as it's currently doing. The app was requesting product details data from the cache server and hit an I/O boundary after a while.

Then the site became unresponsive and it was not clear how to prevent it from doing that. We didn't investigate further since the actual update was supposed to only be minor and we had other fish to fry, hence the rollback. I wasn't able to reproduce the problem locally at the time.

@willkg
Copy link
Contributor Author

willkg commented Jan 27, 2016

Ahh... got it. I didn't understand. I thought you meant it was requesting the JSON files from svn over and over again.

I'll look at how it's getting used and what the caching situation is.

Switching this to not-ready.

@willkg
Copy link
Contributor Author

willkg commented Feb 1, 2016

I spent some time looking into things and decided this doesn't help us at all. I WONTFIXed bug #1238696 which this PR addressed and opened up a new bug which will involve different changes.

Thus, I'm closing this out.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants