{"action":"created","issue":{"url":"https://api.github.com/repos/m-lab/mlab-ns/issues/46","labels_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/46/labels{/name}","comments_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/46/comments","events_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/46/events","html_url":"https://github.com/m-lab/mlab-ns/pull/46","id":118902737,"number":46,"title":"Support for querying against the Maxmind database using the pygeoip module.","user":{"login":"collina","id":582005,"avatar_url":"https://avatars.githubusercontent.com/u/582005?v=3","gravatar_id":"","url":"https://api.github.com/users/collina","html_url":"https://github.com/collina","followers_url":"https://api.github.com/users/collina/followers","following_url":"https://api.github.com/users/collina/following{/other_user}","gists_url":"https://api.github.com/users/collina/gists{/gist_id}","starred_url":"https://api.github.com/users/collina/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/collina/subscriptions","organizations_url":"https://api.github.com/users/collina/orgs","repos_url":"https://api.github.com/users/collina/repos","events_url":"https://api.github.com/users/collina/events{/privacy}","received_events_url":"https://api.github.com/users/collina/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-11-25T19:05:00Z","updated_at":"2015-11-25T19:33:06Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/m-lab/mlab-ns/pulls/46","html_url":"https://github.com/m-lab/mlab-ns/pull/46","diff_url":"https://github.com/m-lab/mlab-ns/pull/46.diff","patch_url":"https://github.com/m-lab/mlab-ns/pull/46.patch"},"body":""},"comment":{"url":"https://api.github.com/repos/m-lab/mlab-ns/issues/comments/159709027","html_url":"https://github.com/m-lab/mlab-ns/pull/46#issuecomment-159709027","issue_url":"https://api.github.com/repos/m-lab/mlab-ns/issues/46","id":159709027,"user":{"login":"mtlynch","id":7783288,"avatar_url":"https://avatars.githubusercontent.com/u/7783288?v=3","gravatar_id":"","url":"https://api.github.com/users/mtlynch","html_url":"https://github.com/mtlynch","followers_url":"https://api.github.com/users/mtlynch/followers","following_url":"https://api.github.com/users/mtlynch/following{/other_user}","gists_url":"https://api.github.com/users/mtlynch/gists{/gist_id}","starred_url":"https://api.github.com/users/mtlynch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mtlynch/subscriptions","organizations_url":"https://api.github.com/users/mtlynch/orgs","repos_url":"https://api.github.com/users/mtlynch/repos","events_url":"https://api.github.com/users/mtlynch/events{/privacy}","received_events_url":"https://api.github.com/users/mtlynch/received_events","type":"User","site_admin":false},"created_at":"2015-11-25T19:33:06Z","updated_at":"2015-11-25T19:33:06Z","body":"Cool, this looks really good. A few high level notes:\r\n\r\n* Can we switch to Reviewable since I expect this to go a few rounds?\r\n* Can we get unit test(s) for `get_ip_geolocation_datafile`? (mocking out `pygeoip.GeoIP`)\r\n* Can we just strip out all of the datastore-based maxmind code? It's pretty messy to have the dead code sitting alongside the live code.\r\n\r\nIf we knew that this was just temporary and that we'd be switching back to Datastore in Q1, I'd be more open to leaving both in to reduce code churn, but I think there's a strong possibility that the long term \"right\" fix is going to get neglected for a while so we'll potentially have dead code and irrelevant tests rotting in the repo for years.\r\n\r\nHave you measured the difference in perf (checking out the GCP logs) of this implementation vs. Datastore-based? The only thing that makes this implementation potentially not the right long term approach is that I expect that reading from the filesystem is significantly slower than reading from the Datastore. But if it's a perf hit of like <10%, I think we're better off long term with pygeoip because it's so much less code/complexity."}}