GitHub PullRequestReviewCommentEvent at 2014-07-09 04:29:37
Repository: juju
Actor: davecheney
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/juju/juju/pulls/comments/14693181","id":14693181,"diff_hunk":"@@ -1537,80 +1538,47 @@ func (st *State) WatchActions() StringsWatcher {\n \treturn newActionWatcher(st)\n }\n \n-// initial pre-loads the actions documents that are already queued for\n-// the units this watcher was started for\n-func (w *actionWatcher) initial() (set.Strings, error) {\n-\tvar actions set.Strings\n-\tvar doc actionDoc\n-\titer := w.st.actions.Find(nil).Iter()\n-\tfor iter.Next(&doc) {\n-\t\tif w.filterFn(doc.Id) {\n-\t\t\tactions.Add(doc.Id)\n-\t\t}\n-\t}\n-\treturn actions, iter.Close()\n-}\n-\n func newActionWatcher(st *State, receivers ...ActionReceiver) StringsWatcher {\n \tw := &actionWatcher{\n \t\tcommonWatcher: commonWatcher{st: st},\n-\t\tout:           make(chan []string),\n+\t\tsource:        make(chan watcher.Change),\n+\t\tsink:          make(chan []string),\n \t}\n \tw.filterFn = w.makeFilter(receivers...)\n \n \tgo func() {\n \t\tdefer w.tomb.Done()\n-\t\tdefer close(w.out)\n+\t\tdefer close(w.sink)\n+\t\tdefer close(w.source)\n \t\tw.tomb.Kill(w.loop())\n \t}()\n \n \treturn w\n }\n \n-// makeFilter constructs a predicate to filter keys that have the prefix matching\n-// one of the passed in ActionReceivers, or returns nil if tags is empty\n-func (w *actionWatcher) makeFilter(receivers ...ActionReceiver) func(interface{}) bool {\n-\tif len(receivers) == 0 {\n-\t\treturn nil\n-\t}\n-\tprefixes := make([]string, len(receivers))\n-\tfor ix, receiver := range receivers {\n-\t\tprefixes[ix] = ensureActionMarker(receiver.Name())\n-\t}\n-\treturn func(key interface{}) bool {\n-\t\tif k, ok := key.(string); ok {\n-\t\t\tfor _, prefix := range prefixes {\n-\t\t\t\tif strings.HasPrefix(k, prefix) {\n-\t\t\t\t\treturn true\n-\t\t\t\t}\n-\t\t\t}\n-\t\t} else {\n-\t\t\tlogger.Warningf(\"actionWatcher got unexpected changes key.  expected string key got %+v\", key)\n-\t\t}\n-\t\treturn false\n-\t}\n-}\n-\n // Changes returns the event channel for w\n func (w *actionWatcher) Changes() <-chan []string {\n-\treturn w.out\n+\treturn w.sink\n }\n \n func (w *actionWatcher) loop() error {\n-\tin := make(chan watcher.Change)\n-\tchanges, err := w.initial()\n+\tvar (\n+\t\tinitial set.Strings\n+\t\tchanges set.Strings\n+\t\tin      <-chan watcher.Change = w.source\n+\t\tout     chan<- []string       = w.sink\n+\t\terr     error\n+\t)\n+\n+\tw.st.watcher.WatchCollectionWithFilter(w.st.actions.Name, w.source, w.filterFn)\n+\tdefer w.st.watcher.UnwatchCollection(w.st.actions.Name, w.source)\n+\n+\tinitial, err = w.initial()","path":"state/watcher.go","position":92,"original_position":92,"commit_id":"9b055e25198db94f57139d073c8db3d05e618882","original_commit_id":"9b055e25198db94f57139d073c8db3d05e618882","user":{"login":"davecheney","id":7171,"avatar_url":"https://avatars.githubusercontent.com/u/7171?","gravatar_id":"fda08117336cfde6562315df04b976e8","url":"https://api.github.com/users/davecheney","html_url":"https://github.com/davecheney","followers_url":"https://api.github.com/users/davecheney/followers","following_url":"https://api.github.com/users/davecheney/following{/other_user}","gists_url":"https://api.github.com/users/davecheney/gists{/gist_id}","starred_url":"https://api.github.com/users/davecheney/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/davecheney/subscriptions","organizations_url":"https://api.github.com/users/davecheney/orgs","repos_url":"https://api.github.com/users/davecheney/repos","events_url":"https://api.github.com/users/davecheney/events{/privacy}","received_events_url":"https://api.github.com/users/davecheney/received_events","type":"User","site_admin":false},"body":"use short decl here","created_at":"2014-07-09T04:29:37Z","updated_at":"2014-07-09T04:29:37Z","html_url":"https://github.com/juju/juju/pull/275#discussion_r14693181","pull_request_url":"https://api.github.com/repos/juju/juju/pulls/275","_links":{"self":{"href":"https://api.github.com/repos/juju/juju/pulls/comments/14693181"},"html":{"href":"https://github.com/juju/juju/pull/275#discussion_r14693181"},"pull_request":{"href":"https://api.github.com/repos/juju/juju/pulls/275"}}},"pull_request":{"url":"https://api.github.com/repos/juju/juju/pulls/275","id":18139791,"html_url":"https://github.com/juju/juju/pull/275","diff_url":"https://github.com/juju/juju/pull/275.diff","patch_url":"https://github.com/juju/juju/pull/275.patch","issue_url":"https://api.github.com/repos/juju/juju/issues/275","number":275,"state":"open","title":"Actions :: actionWatcher consume initial events","user":{"login":"johnweldon","id":51034,"avatar_url":"https://avatars.githubusercontent.com/u/51034?","gravatar_id":"262d914b9ff3f5b7f599de600cf463a8","url":"https://api.github.com/users/johnweldon","html_url":"https://github.com/johnweldon","followers_url":"https://api.github.com/users/johnweldon/followers","following_url":"https://api.github.com/users/johnweldon/following{/other_user}","gists_url":"https://api.github.com/users/johnweldon/gists{/gist_id}","starred_url":"https://api.github.com/users/johnweldon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/johnweldon/subscriptions","organizations_url":"https://api.github.com/users/johnweldon/orgs","repos_url":"https://api.github.com/users/johnweldon/repos","events_url":"https://api.github.com/users/johnweldon/events{/privacy}","received_events_url":"https://api.github.com/users/johnweldon/received_events","type":"User","site_admin":false},"body":"Context\r\n---\r\nWhen a `watcher` is started it registers with the `state.watcher` to be notified when changelog items matching it's filter are detected.  The `state.watcher` does this by periodically looking through all the new changes in the mongo changelog and feeding appropriate events to all of the registered watchers.\r\n\r\nWhen an `actionWatcher` is first started, we want to not only be notified of new actions being added to the `state.actions` collection, but also of any actions added to the `state.actions` collection before the `actionWatcher` was started.\r\n\r\nThis is done by querying the state.actions collection in the initialization of the `actionWatcher`, and pre-loading those actionId's into the changes buffer before the watcher loop starts.\r\n\r\nThis is a common idiom in several of the watchers.\r\n\r\n\r\nProblem\r\n---\r\nIt is possible when the `state.watcher` has been running for more than a few seconds (i.e. in long running tests, or the real world, but not in normal short running tests), that between the time the `actionWatcher` is started, and the `state.watcher` begins notifying the `actionWatcher` of changes, that an action may be added to the `state.actions` collection.\r\n\r\nIn this case the pre-loaded actionId's could be sent once from the initially loaded changes buffer, and then again when the `state.watcher` detects and notifies the `actionWatcher` of that addition.\r\n\r\nHowever, this is somewhat race-ey.  If the timing is different and the goroutines run in a different order then the pre-loaded actionId's will only be sent down once.\r\n\r\n\r\nThis PR's Solution\r\n---\r\nIn this PR we are caching only those pre-loaded actionId's and then filtering subsequent notifications by removing these specific actionId's, since we know they have already been sent.\r\n\r\nThis is not too bad of a solution; we're not caching *all* of the previously seen actionId's, only the ones vulnerable to this race condition.\r\n\r\nThe alternative seems way worse, which would be to ask the `state.watcher` what the last changelog number it processed was when we initially register the watcher, and then only pre-loading actionIds that came before that changelog number.  I don't like that option because we're exposing previously internal details of the `state.Watcher` to the `actionWatcher`.","created_at":"2014-07-09T03:02:33Z","updated_at":"2014-07-09T04:29:37Z","closed_at":null,"merged_at":null,"merge_commit_sha":"3e45e6945fe040a1cdc3b28d3a73eb10cbfc4744","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/juju/juju/pulls/275/commits","review_comments_url":"https://api.github.com/repos/juju/juju/pulls/275/comments","review_comment_url":"https://api.github.com/repos/juju/juju/pulls/comments/{number}","comments_url":"https://api.github.com/repos/juju/juju/issues/275/comments","statuses_url":"https://api.github.com/repos/juju/juju/statuses/9b055e25198db94f57139d073c8db3d05e618882","head":{"label":"johnweldon:actionwatcher-initial-events","ref":"actionwatcher-initial-events","sha":"9b055e25198db94f57139d073c8db3d05e618882","user":{"login":"johnweldon","id":51034,"avatar_url":"https://avatars.githubusercontent.com/u/51034?","gravatar_id":"262d914b9ff3f5b7f599de600cf463a8","url":"https://api.github.com/users/johnweldon","html_url":"https://github.com/johnweldon","followers_url":"https://api.github.com/users/johnweldon/followers","following_url":"https://api.github.com/users/johnweldon/following{/other_user}","gists_url":"https://api.github.com/users/johnweldon/gists{/gist_id}","starred_url":"https://api.github.com/users/johnweldon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/johnweldon/subscriptions","organizations_url":"https://api.github.com/users/johnweldon/orgs","repos_url":"https://api.github.com/users/johnweldon/repos","events_url":"https://api.github.com/users/johnweldon/events{/privacy}","received_events_url":"https://api.github.com/users/johnweldon/received_events","type":"User","site_admin":false},"repo":{"id":20453955,"name":"juju","full_name":"johnweldon/juju","owner":{"login":"johnweldon","id":51034,"avatar_url":"https://avatars.githubusercontent.com/u/51034?","gravatar_id":"262d914b9ff3f5b7f599de600cf463a8","url":"https://api.github.com/users/johnweldon","html_url":"https://github.com/johnweldon","followers_url":"https://api.github.com/users/johnweldon/followers","following_url":"https://api.github.com/users/johnweldon/following{/other_user}","gists_url":"https://api.github.com/users/johnweldon/gists{/gist_id}","starred_url":"https://api.github.com/users/johnweldon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/johnweldon/subscriptions","organizations_url":"https://api.github.com/users/johnweldon/orgs","repos_url":"https://api.github.com/users/johnweldon/repos","events_url":"https://api.github.com/users/johnweldon/events{/privacy}","received_events_url":"https://api.github.com/users/johnweldon/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/johnweldon/juju","description":"juju is devops distilled","fork":true,"url":"https://api.github.com/repos/johnweldon/juju","forks_url":"https://api.github.com/repos/johnweldon/juju/forks","keys_url":"https://api.github.com/repos/johnweldon/juju/keys{/key_id}","collaborators_url":"https://api.github.com/repos/johnweldon/juju/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/johnweldon/juju/teams","hooks_url":"https://api.github.com/repos/johnweldon/juju/hooks","issue_events_url":"https://api.github.com/repos/johnweldon/juju/issues/events{/number}","events_url":"https://api.github.com/repos/johnweldon/juju/events","assignees_url":"https://api.github.com/repos/johnweldon/juju/assignees{/user}","branches_url":"https://api.github.com/repos/johnweldon/juju/branches{/branch}","tags_url":"https://api.github.com/repos/johnweldon/juju/tags","blobs_url":"https://api.github.com/repos/johnweldon/juju/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/johnweldon/juju/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/johnweldon/juju/git/refs{/sha}","trees_url":"https://api.github.com/repos/johnweldon/juju/git/trees{/sha}","statuses_url":"https://api.github.com/repos/johnweldon/juju/statuses/{sha}","languages_url":"https://api.github.com/repos/johnweldon/juju/languages","stargazers_url":"https://api.github.com/repos/johnweldon/juju/stargazers","contributors_url":"https://api.github.com/repos/johnweldon/juju/contributors","subscribers_url":"https://api.github.com/repos/johnweldon/juju/subscribers","subscription_url":"https://api.github.com/repos/johnweldon/juju/subscription","commits_url":"https://api.github.com/repos/johnweldon/juju/commits{/sha}","git_commits_url":"https://api.github.com/repos/johnweldon/juju/git/commits{/sha}","comments_url":"https://api.github.com/repos/johnweldon/juju/comments{/number}","issue_comment_url":"https://api.github.com/repos/johnweldon/juju/issues/comments/{number}","contents_url":"https://api.github.com/repos/johnweldon/juju/contents/{+path}","compare_url":"https://api.github.com/repos/johnweldon/juju/compare/{base}...{head}","merges_url":"https://api.github.com/repos/johnweldon/juju/merges","archive_url":"https://api.github.com/repos/johnweldon/juju/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/johnweldon/juju/downloads","issues_url":"https://api.github.com/repos/johnweldon/juju/issues{/number}","pulls_url":"https://api.github.com/repos/johnweldon/juju/pulls{/number}","milestones_url":"https://api.github.com/repos/johnweldon/juju/milestones{/number}","notifications_url":"https://api.github.com/repos/johnweldon/juju/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/johnweldon/juju/labels{/name}","releases_url":"https://api.github.com/repos/johnweldon/juju/releases{/id}","created_at":"2014-06-03T18:17:12Z","updated_at":"2014-06-25T17:58:16Z","pushed_at":"2014-07-09T03:14:38Z","git_url":"git://github.com/johnweldon/juju.git","ssh_url":"git@github.com:johnweldon/juju.git","clone_url":"https://github.com/johnweldon/juju.git","svn_url":"https://github.com/johnweldon/juju","homepage":null,"size":25112,"stargazers_count":0,"watchers_count":0,"language":"Go","has_issues":false,"has_downloads":true,"has_wiki":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"juju:master","ref":"master","sha":"16af08da69cf377d4923b9f53f1c6943ecb63eae","user":{"login":"juju","id":4604548,"avatar_url":"https://avatars.githubusercontent.com/u/4604548?","gravatar_id":"b095d1e26e5705abe0ad2de7d87aac2e","url":"https://api.github.com/users/juju","html_url":"https://github.com/juju","followers_url":"https://api.github.com/users/juju/followers","following_url":"https://api.github.com/users/juju/following{/other_user}","gists_url":"https://api.github.com/users/juju/gists{/gist_id}","starred_url":"https://api.github.com/users/juju/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/juju/subscriptions","organizations_url":"https://api.github.com/users/juju/orgs","repos_url":"https://api.github.com/users/juju/repos","events_url":"https://api.github.com/users/juju/events{/privacy}","received_events_url":"https://api.github.com/users/juju/received_events","type":"Organization","site_admin":false},"repo":{"id":20445947,"name":"juju","full_name":"juju/juju","owner":{"login":"juju","id":4604548,"avatar_url":"https://avatars.githubusercontent.com/u/4604548?","gravatar_id":"b095d1e26e5705abe0ad2de7d87aac2e","url":"https://api.github.com/users/juju","html_url":"https://github.com/juju","followers_url":"https://api.github.com/users/juju/followers","following_url":"https://api.github.com/users/juju/following{/other_user}","gists_url":"https://api.github.com/users/juju/gists{/gist_id}","starred_url":"https://api.github.com/users/juju/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/juju/subscriptions","organizations_url":"https://api.github.com/users/juju/orgs","repos_url":"https://api.github.com/users/juju/repos","events_url":"https://api.github.com/users/juju/events{/privacy}","received_events_url":"https://api.github.com/users/juju/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/juju/juju","description":"juju is devops distilled","fork":false,"url":"https://api.github.com/repos/juju/juju","forks_url":"https://api.github.com/repos/juju/juju/forks","keys_url":"https://api.github.com/repos/juju/juju/keys{/key_id}","collaborators_url":"https://api.github.com/repos/juju/juju/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/juju/juju/teams","hooks_url":"https://api.github.com/repos/juju/juju/hooks","issue_events_url":"https://api.github.com/repos/juju/juju/issues/events{/number}","events_url":"https://api.github.com/repos/juju/juju/events","assignees_url":"https://api.github.com/repos/juju/juju/assignees{/user}","branches_url":"https://api.github.com/repos/juju/juju/branches{/branch}","tags_url":"https://api.github.com/repos/juju/juju/tags","blobs_url":"https://api.github.com/repos/juju/juju/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/juju/juju/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/juju/juju/git/refs{/sha}","trees_url":"https://api.github.com/repos/juju/juju/git/trees{/sha}","statuses_url":"https://api.github.com/repos/juju/juju/statuses/{sha}","languages_url":"https://api.github.com/repos/juju/juju/languages","stargazers_url":"https://api.github.com/repos/juju/juju/stargazers","contributors_url":"https://api.github.com/repos/juju/juju/contributors","subscribers_url":"https://api.github.com/repos/juju/juju/subscribers","subscription_url":"https://api.github.com/repos/juju/juju/subscription","commits_url":"https://api.github.com/repos/juju/juju/commits{/sha}","git_commits_url":"https://api.github.com/repos/juju/juju/git/commits{/sha}","comments_url":"https://api.github.com/repos/juju/juju/comments{/number}","issue_comment_url":"https://api.github.com/repos/juju/juju/issues/comments/{number}","contents_url":"https://api.github.com/repos/juju/juju/contents/{+path}","compare_url":"https://api.github.com/repos/juju/juju/compare/{base}...{head}","merges_url":"https://api.github.com/repos/juju/juju/merges","archive_url":"https://api.github.com/repos/juju/juju/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/juju/juju/downloads","issues_url":"https://api.github.com/repos/juju/juju/issues{/number}","pulls_url":"https://api.github.com/repos/juju/juju/pulls{/number}","milestones_url":"https://api.github.com/repos/juju/juju/milestones{/number}","notifications_url":"https://api.github.com/repos/juju/juju/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/juju/juju/labels{/name}","releases_url":"https://api.github.com/repos/juju/juju/releases{/id}","created_at":"2014-06-03T14:28:10Z","updated_at":"2014-07-06T23:12:43Z","pushed_at":"2014-07-09T03:29:48Z","git_url":"git://github.com/juju/juju.git","ssh_url":"git@github.com:juju/juju.git","clone_url":"https://github.com/juju/juju.git","svn_url":"https://github.com/juju/juju","homepage":"juju.ubuntu.com","size":30758,"stargazers_count":176,"watchers_count":176,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":40,"mirror_url":null,"open_issues_count":39,"forks":40,"open_issues":39,"watchers":176,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/juju/juju/pulls/275"},"html":{"href":"https://github.com/juju/juju/pull/275"},"issue":{"href":"https://api.github.com/repos/juju/juju/issues/275"},"comments":{"href":"https://api.github.com/repos/juju/juju/issues/275/comments"},"review_comments":{"href":"https://api.github.com/repos/juju/juju/pulls/275/comments"},"review_comment":{"href":"https://api.github.com/repos/juju/juju/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/juju/juju/pulls/275/commits"},"statuses":{"href":"https://api.github.com/repos/juju/juju/statuses/9b055e25198db94f57139d073c8db3d05e618882"}}}}