{"action":"created","issue":{"url":"https://api.github.com/repos/juju/juju/issues/7531","repository_url":"https://api.github.com/repos/juju/juju","labels_url":"https://api.github.com/repos/juju/juju/issues/7531/labels{/name}","comments_url":"https://api.github.com/repos/juju/juju/issues/7531/comments","events_url":"https://api.github.com/repos/juju/juju/issues/7531/events","html_url":"https://github.com/juju/juju/pull/7531","id":237469259,"number":7531,"title":"WIP Batch Pings() before writing them to the Database","user":{"login":"jameinel","id":202877,"avatar_url":"https://avatars3.githubusercontent.com/u/202877?v=3","gravatar_id":"","url":"https://api.github.com/users/jameinel","html_url":"https://github.com/jameinel","followers_url":"https://api.github.com/users/jameinel/followers","following_url":"https://api.github.com/users/jameinel/following{/other_user}","gists_url":"https://api.github.com/users/jameinel/gists{/gist_id}","starred_url":"https://api.github.com/users/jameinel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jameinel/subscriptions","organizations_url":"https://api.github.com/users/jameinel/orgs","repos_url":"https://api.github.com/users/jameinel/repos","events_url":"https://api.github.com/users/jameinel/events{/privacy}","received_events_url":"https://api.github.com/users/jameinel/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2017-06-21T09:23:05Z","updated_at":"2017-06-21T16:12:52Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/juju/juju/pulls/7531","html_url":"https://github.com/juju/juju/pull/7531","diff_url":"https://github.com/juju/juju/pull/7531.diff","patch_url":"https://github.com/juju/juju/pull/7531.patch"},"body":"## Description of change\r\n\r\nIn our existing code, every Ping() that is being driven on every agent causes us to do a Session.Copy() and send a `$inc` to the database. However, its fairly obvious that we could aggregate those pings at some interval. This has some big wins:\r\n1) Because we are setting bits inside integers, we can OR those bit flips in memory and only send a single \"$inc\" to the database. (So if we have 50 pings for the same slot, we can send a single $inc rather than 50 $inc).\r\n2) This also helps when hitting different fields. We still have to set values for all of those fields, but we can do so for all fields that we see in one pass. (it doesn't change the number of $inc, but it does change the number of times we touch the same document.)\r\n3) The code is written to handle pings across model UUIDs, but we currently create a PingBatcher for each State. This is less of a win because the content is stored in different documents anyway. It would still probably be good, but I'm happy with 1 & 2.\r\n4) Should help tremendously with thundering-herd. The existing Ping() code did a Session.Copy() for every Ping. While many of those will go back into the pool, that still gives a very wide number of concurrent requests possible. (2500 agents => 2500 potential concurrent requests.)\r\nWith this, we should significantly reduce contention, because we can handle 2500 concurrent requests as just a mutex + map update, and then 1s later actually flush all of those requests to the database in a single update.\r\n\r\n5) The Pruner test \"TestDeepStressStaysSane\" exists to test our Pruning code plays nicely when lots of pingers are active. That test shows really good (if slightly artificial) results. At 500 pingers (actuall 1k concurrent), the test drops from 2s to 1.5s total time. At 5000 pingers, the test scales to about 18s, and at 10,000 pingers it goes to 37s (essentially linear). With 5000 pingers and the old code, the test never gets past the first loop after 10 minutes. I think that's mostly a mongo socket contention as it doesn't like 5k (10k?) concurrent sessions.\r\n\r\nWe also expose this as a per-model configuration knob. So users with heavily loaded models can trade off update frequency for performance, though ultimately I'm hoping we'll find a default value that works well and people won't have to touch it.\r\n\r\nThis is currently WIP, because:\r\na) There are some tests that assume we see agents coming alive right away. I need to do something to tweak (either forcibly flush, or be able to increase the frequency of flushing.)\r\nb) I want to do real performance testing to find a sweet spot of 'lowest time that still gives good results\". It may be 100ms, or it may be 10s. Since our ping windows are 30s anyway (and it has to be dead for 2 windows to be considered dead), we have some pretty large batching windows. But I also have the feeling that we'll hit diminishing returns.\r\nc) I'd like to evaluate using $bit operations instead of $inc, which would make it safe if we ever did ping the same slot twice. (Arguably we could simplify a lot of this if we did.)\r\n\r\n## QA steps\r\n\r\nThis shouldn't change users interaction with the system. So its mostly that \"CI continues to pass all tests\". We *should* see scale testing show less database operations being run on the presence.presence.pings collection, and a decrease in things like \"mongotop\" of that being a high impact collection.\r\n\r\n## Documentation changes\r\n\r\nNothing to the user workflow. Only real documentation is performance improvements.\r\n\r\n## Bug reference\r\n\r\n[lp:1697519](https://bugs.launchpad.net/juju/+bug/1697519)"},"comment":{"url":"https://api.github.com/repos/juju/juju/issues/comments/310129225","html_url":"https://github.com/juju/juju/pull/7531#issuecomment-310129225","issue_url":"https://api.github.com/repos/juju/juju/issues/7531","id":310129225,"user":{"login":"jameinel","id":202877,"avatar_url":"https://avatars3.githubusercontent.com/u/202877?v=3","gravatar_id":"","url":"https://api.github.com/users/jameinel","html_url":"https://github.com/jameinel","followers_url":"https://api.github.com/users/jameinel/followers","following_url":"https://api.github.com/users/jameinel/following{/other_user}","gists_url":"https://api.github.com/users/jameinel/gists{/gist_id}","starred_url":"https://api.github.com/users/jameinel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jameinel/subscriptions","organizations_url":"https://api.github.com/users/jameinel/orgs","repos_url":"https://api.github.com/users/jameinel/repos","events_url":"https://api.github.com/users/jameinel/events{/privacy}","received_events_url":"https://api.github.com/users/jameinel/received_events","type":"User","site_admin":false},"created_at":"2017-06-21T16:12:52Z","updated_at":"2017-06-21T16:12:52Z","body":"Given the lack of real effect from changing the ping interval, I would consider just removing the config option. Its really not worth tweaking the knob.\r\nIf anyone wants, we could trivially pick a 100ms or 5s or whatever. I think I'm very comfortable with 1s as a predictable number that provides all the benefits we want.\r\n"}}
{"actor":{"display_login":"jameinel"}}