{"action":"created","issue":{"url":"https://api.github.com/repos/Shopify/identity_cache/issues/274","repository_url":"https://api.github.com/repos/Shopify/identity_cache","labels_url":"https://api.github.com/repos/Shopify/identity_cache/issues/274/labels{/name}","comments_url":"https://api.github.com/repos/Shopify/identity_cache/issues/274/comments","events_url":"https://api.github.com/repos/Shopify/identity_cache/issues/274/events","html_url":"https://github.com/Shopify/identity_cache/issues/274","id":159228480,"number":274,"title":"Mark IDC fetched records as read_only","user":{"login":"dylanahsmith","id":954402,"avatar_url":"https://avatars.githubusercontent.com/u/954402?v=3","gravatar_id":"","url":"https://api.github.com/users/dylanahsmith","html_url":"https://github.com/dylanahsmith","followers_url":"https://api.github.com/users/dylanahsmith/followers","following_url":"https://api.github.com/users/dylanahsmith/following{/other_user}","gists_url":"https://api.github.com/users/dylanahsmith/gists{/gist_id}","starred_url":"https://api.github.com/users/dylanahsmith/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dylanahsmith/subscriptions","organizations_url":"https://api.github.com/users/dylanahsmith/orgs","repos_url":"https://api.github.com/users/dylanahsmith/repos","events_url":"https://api.github.com/users/dylanahsmith/events{/privacy}","received_events_url":"https://api.github.com/users/dylanahsmith/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2016-06-08T17:39:47Z","updated_at":"2016-06-23T07:51:10Z","closed_at":null,"body":"cc @airhorns, @camilo\r\n\r\nThis was attempted before in https://github.com/Shopify/identity_cache/pull/85, which gives a good explanation for the problem:\r\n\r\n> IdentityCache is a forever inconsistent datastore. It is a duplication of state across two different storage backends, and unless we somehow confirmed commit in both places before acknowledging, we can't guarantee that the two are in sync. The ruby process could die before it has a chance to issue the memcache DEL, the request could get lost if the connection is interrupted or any number of other things could prevent IDC from reflecting whats truly in the database.\r\n>\r\n> This being the case, it is sketchy to write something you find in IDC back to the database, because you might be writing out of date data to the database. This is already the case with a MySQL insert in Rails cause you had to have read that data a finite amount of time ago, but that window is usually much smaller, and we have locking primitives and transactions to manage this. With IDC, we don't have that, and we don't have any way of knowing if the data is out of date without asking the database, which entirely defeats the purpose of IDC.\r\n>\r\n> So, I suggest we return only readonly records from IDC. If you are gonna write a record, first fetch its values from the DB.\r\n\r\nOne thing I think we should do differently from the pull request, is that records returned from IDC methods when `IdentityCache.should_cache?` returns false should not be marked as read-only.  The problem with marking those records as read-only is that it prevents us from sharing code to use for reading and writing, e.g. calculating some aggregation for a confirmation page which should be consistent with the calculation for the operation itself.\r\n\r\nOn the other hand, records fetched from the database on a cache miss when `IdentityCache.should_cache?` returns true should be marked as read-only.  This way code that does the wrong thing will fail deterministically, rather than passing on a cache miss and failing on a cache hit.\r\n\r\nWe might also want to consider making this behaviour configurable, so that we don't get blocked on Shopify needed to be updated to work with this new behaviour, which was probably the main reason that the previous PR didn't get finished and merged.  That way we can at least have identity cache do the right thing by default on the next release with breaking changes."},"comment":{"url":"https://api.github.com/repos/Shopify/identity_cache/issues/comments/227975692","html_url":"https://github.com/Shopify/identity_cache/issues/274#issuecomment-227975692","issue_url":"https://api.github.com/repos/Shopify/identity_cache/issues/274","id":227975692,"user":{"login":"dylanahsmith","id":954402,"avatar_url":"https://avatars.githubusercontent.com/u/954402?v=3","gravatar_id":"","url":"https://api.github.com/users/dylanahsmith","html_url":"https://github.com/dylanahsmith","followers_url":"https://api.github.com/users/dylanahsmith/followers","following_url":"https://api.github.com/users/dylanahsmith/following{/other_user}","gists_url":"https://api.github.com/users/dylanahsmith/gists{/gist_id}","starred_url":"https://api.github.com/users/dylanahsmith/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dylanahsmith/subscriptions","organizations_url":"https://api.github.com/users/dylanahsmith/orgs","repos_url":"https://api.github.com/users/dylanahsmith/repos","events_url":"https://api.github.com/users/dylanahsmith/events{/privacy}","received_events_url":"https://api.github.com/users/dylanahsmith/received_events","type":"User","site_admin":false},"created_at":"2016-06-23T07:51:10Z","updated_at":"2016-06-23T07:51:10Z","body":"Thanks for pointing that out, since we have a `IdentityCache.should_use_cache?` monkey patch in Shopify which not only checks multiple active database connections for open transactions, but also ignores the transactional fixture transaction.  Perhaps we should bring over that check into this repo so tests behave like production by default."}}
{"actor":{"display_login":"dylanahsmith"}}