{"action":"created","issue":{"url":"https://api.github.com/repos/travis-ci/travis-scheduler/issues/126","repository_url":"https://api.github.com/repos/travis-ci/travis-scheduler","labels_url":"https://api.github.com/repos/travis-ci/travis-scheduler/issues/126/labels{/name}","comments_url":"https://api.github.com/repos/travis-ci/travis-scheduler/issues/126/comments","events_url":"https://api.github.com/repos/travis-ci/travis-scheduler/issues/126/events","html_url":"https://github.com/travis-ci/travis-scheduler/pull/126","id":298542929,"number":126,"title":"Don't throw exception when checking if job is enqueable by owner","user":{"login":"bogdanap","id":1673649,"avatar_url":"https://avatars3.githubusercontent.com/u/1673649?v=4","gravatar_id":"","url":"https://api.github.com/users/bogdanap","html_url":"https://github.com/bogdanap","followers_url":"https://api.github.com/users/bogdanap/followers","following_url":"https://api.github.com/users/bogdanap/following{/other_user}","gists_url":"https://api.github.com/users/bogdanap/gists{/gist_id}","starred_url":"https://api.github.com/users/bogdanap/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bogdanap/subscriptions","organizations_url":"https://api.github.com/users/bogdanap/orgs","repos_url":"https://api.github.com/users/bogdanap/repos","events_url":"https://api.github.com/users/bogdanap/events{/privacy}","received_events_url":"https://api.github.com/users/bogdanap/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-02-20T10:32:32Z","updated_at":"2018-02-20T10:51:22Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/travis-ci/travis-scheduler/pulls/126","html_url":"https://github.com/travis-ci/travis-scheduler/pull/126","diff_url":"https://github.com/travis-ci/travis-scheduler/pull/126.diff","patch_url":"https://github.com/travis-ci/travis-scheduler/pull/126.patch"},"body":"I'm having a bit of trouble understanding why we need to throw the :limited result for the ByOwner limit check, so I thought I'd use this PR to ask. 😄 \r\n\r\nAs far as I understand it, we're skipping the job regardless if public_mode is true or false (we just throw the exception in one case and return false in the other). \r\nI expect it might have some effect on the rest of the jobs (since we `break` when encountering the `:limited` result), but it's not very clear to me why that would be and if that is indeed the case, it would be good to have some unit tests covering it."},"comment":{"url":"https://api.github.com/repos/travis-ci/travis-scheduler/issues/comments/366939915","html_url":"https://github.com/travis-ci/travis-scheduler/pull/126#issuecomment-366939915","issue_url":"https://api.github.com/repos/travis-ci/travis-scheduler/issues/126","id":366939915,"user":{"login":"igorwwwwwwwwwwwwwwwwwwww","id":11158255,"avatar_url":"https://avatars1.githubusercontent.com/u/11158255?v=4","gravatar_id":"","url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww","html_url":"https://github.com/igorwwwwwwwwwwwwwwwwwwww","followers_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/followers","following_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/following{/other_user}","gists_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/gists{/gist_id}","starred_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/subscriptions","organizations_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/orgs","repos_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/repos","events_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/events{/privacy}","received_events_url":"https://api.github.com/users/igorwwwwwwwwwwwwwwwwwwww/received_events","type":"User","site_admin":false},"created_at":"2018-02-20T10:51:22Z","updated_at":"2018-02-20T10:51:22Z","author_association":"CONTRIBUTOR","body":"I think your reasoning is solid. it seems like the main reason for the `throw` is indeed the `break` which will stop enqueueing any further jobs. My guess is that this is an optimization: once the first job has been limited by owner concurrency, we don't want to continue scheduling any other jobs, because all the following jobs will also be implicitly limited by concurrency.\r\n\r\nI would not expect this to make a difference in terms of correctness, but not breaking out of the loop may have implications for performance. we should check to see if the other checks are doing anything expensive on every iteration.\r\n\r\nthe fact that we break out of the loop early in fact means we have no information about why the rest of the jobs were not scheduled. which is something that we actually _want_ to know for upselling and https://github.com/travis-ci/reliability/issues/5, since we can only upsell based on jobs that _would have run_ -- in other words, jobs that were limited _exclusively_ by concurrency."}}
{"actor":{"display_login":"igorwwwwwwwwwwwwwwwwwwww"}}