)]}' { "commit": "eb343023b7a26b66a3c356da75958bb7db3f2112", "tree": "fb0c722aa6f1962b73bc0f8dcce3ab6a1cb1dbe6", "parents": [ "3e04e87fc571b0a66765550612e4fb7a23e1755f" ], "author": { "name": "Edwin Kempin", "email": "ekempin@google.com", "time": "Wed Oct 07 15:54:13 2020 +0200" }, "committer": { "name": "Edwin Kempin", "email": "ekempin@google.com", "time": "Mon Oct 12 12:19:38 2020 +0200" }, "message": "Avoid loading all accounts when code ownership is assigned to all users\n\nWhen a code ownership was assigned to all users by using the \u0027*\u0027\nwildcard as email, CodeOwnerResolver was loading all accounts. Loading\nall accounts requires parsing all user refs. Then we checked the\nvisibility of all accounts, which means that all accounts were retrieved\nfrom the cache and, depending on the accounts visibility setting, groups\nfor all users had to be loaded. Doing this is too expensive and if\ncollection of performance logs is enabled this can lead to memory\nexceeded errors because each account lookup creates performance logs\nwhich are collected in memory.\n\nTo fix this, CodeOwnerResolver is no longer resolving \u0027*\u0027 to all users.\nInstead we return a CodeOwnerResolverResult that contains all resolved\ncode owners plus a flag telling whether all users are code owners.\nReturning this as a flag, rather than returning all accounts, is much\ncheaper, but it means that callers now must handle this case on their\nown. However they can do something smarter than loading all accounts.\n\nThere are 2 callers for which this is relevant:\n\n1. CodeOwnerApprovalCheck:\n Here we need to determine whether a code owner approved the change,\n or whether a code owner is a reviewer of the change. If a file is\n owned by all users and the approver set is non-empty, we know that\n the file is approved (since there is at least one approver and any\n user is a code owner). If there is no approval but a reviewer, we\n know that the status for the file is pending (since all users are a\n code owner, the reviewer must be a code owner).\n\n2. AbstractGetCodeOwnersForPath:\n Here we list code owners, but listing of code owners is always done\n with a limit (if no limit is specified, there is a default limit).\n If all users are code owners we want to include random users until\n the limit is reached. For this we asked for 2 times the number of\n users that we still need to complete the suggestion list. We use the\n factor 2 here because some users may still be filtered out (e.g.\n because they are service users or because they cannot see the\n branch). When selecting random users for the suggestion we must\n ensure that the included users are visible to the caller. This means\n the logic that picks the random users depends on the configured\n account visibility:\n\n * ALL:\n If all accounts are visible, we can randomly pick any account. For\n this we retrieve the list of all accounts (getting this should be\n rather cheap) and select some random entries.\n * NONE:\n If no other account is visible, we return an empty list.\n * SAME_GROUP / VISIBLE_GROUP:\n These are tricky, because potentially we will again end up loading\n all accounts and all groups until we find enough visible accounts.\n To avoid this we do a best effort to find enough accounts. We pick\n a random sample of users that is 3 times the limit and filter out\n non-visible accounts, in the hope that the sample contained enough\n visible accounts to reach our limit. If we didn\u0027t find enough\n visible accounts, we return less code owners than the requested\n limit, although there might be further code owners available. This\n is a compromise that we have to accept in order to ensure good\n performance.\n\n In addition, if the calling user can view all accounts (via the\n VIEW_ALL_ACCOUNTS global capability) we always apply the same logic as\n for the ALL account visibility setting.\n\nChange-Id: I896edd3a08230106316b327e84f18fab239f4503\nSigned-off-by: Edwin Kempin \u003cekempin@google.com\u003e\n", "tree_diff": [ { "type": "modify", "old_id": "4ed19ced506498fbc26fe1c54acb29b9f20f8d77", "old_mode": 33188, "old_path": "java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java", "new_id": "a49620b657c283a65ccd932113330be7e08117dc", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java" }, { "type": "modify", "old_id": "b53afb24c171d97d5ede00ac066dbd7fb416526c", "old_mode": 33188, "old_path": "java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java", "new_id": "fdd4da210dc5613733962b42cff1e2be8dfe4b79", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java" }, { "type": "add", "old_id": "0000000000000000000000000000000000000000", "old_mode": 0, "old_path": "/dev/null", "new_id": "226d5417b31c3915e8f8a8468f7422512f1abfed", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverResult.java" }, { "type": "modify", "old_id": "dd4fd6a6fa06061f70a1c936c0cbf2d3f6604f1a", "old_mode": 33188, "old_path": "java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java", "new_id": "fcc55432108d4c75dab5f15d21ae07b49ee4a679", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java" }, { "type": "modify", "old_id": "baf260d84d8b7f3fe03629a26d31502359a2be96", "old_mode": 33188, "old_path": "java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java", "new_id": "6e3d054414cdc3d8cec3f3e04b928ab8ac61358f", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java" }, { "type": "modify", "old_id": "43a5d2133c6657e86db3f930808efbfa3210858b", "old_mode": 33188, "old_path": "java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java", "new_id": "348361445163897894bd36fde5d29ee76a4127ce", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java" }, { "type": "modify", "old_id": "4e883c5bd264cd76b523d846cd0b9a74dcad4f43", "old_mode": 33188, "old_path": "java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java", "new_id": "f0023ff26e21e0fa5a697bb10afe88e51d7f5a28", "new_mode": 33188, "new_path": "java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java" }, { "type": "modify", "old_id": "360e2a7aa2794496a780a50be34234e07396d23b", "old_mode": 33188, "old_path": "javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java", "new_id": "4048684f64173107534e3bfe0fabc4ef9707a4e2", "new_mode": 33188, "new_path": "javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java" }, { "type": "modify", "old_id": "861a20f7bf2d6b9a618403c47282c24c1946c431", "old_mode": 33188, "old_path": "javatests/com/google/gerrit/plugins/codeowners/acceptance/api/BUILD", "new_id": "48211213b0ffa09a3ce44afea0db7f7fc9fcdf36", "new_mode": 33188, "new_path": "javatests/com/google/gerrit/plugins/codeowners/acceptance/api/BUILD" }, { "type": "modify", "old_id": "69e5f976bb11a0b95d9219d5a3dc303c09c67c19", "old_mode": 33188, "old_path": "javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java", "new_id": "dc2549f85d6d6f9ecfbcc92644f10b6101351484", "new_mode": 33188, "new_path": "javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheckTest.java" }, { "type": "modify", "old_id": "8737bc65825dafb4364a4c71d3b48ada39d6b554", "old_mode": 33188, "old_path": "javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java", "new_id": "f390336ffc3e2e9ac5217cce685a863d9113ddfb", "new_mode": 33188, "new_path": "javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java" } ] }