Skip to content

Further improved ACL algorithm for picking categories#7966

Merged
madolson merged 2 commits intoredis:unstablefrom
madolson:acl-fix-again
Oct 28, 2020
Merged

Further improved ACL algorithm for picking categories#7966
madolson merged 2 commits intoredis:unstablefrom
madolson:acl-fix-again

Conversation

@madolson
Copy link
Contributor

@madolson madolson commented Oct 27, 2020

Built a slight better algorithm that picks categories that are closer to optimal since there were still some basic cases that were producing non-obvious results. Was impacting someone else's PR.

Note, we do need to check both maxsame and mindiff for it to pass the test cases, otherwise several cases fail. e.g.

Expected '+@all -@slow' to be equal to '+@all -@geo -@scripting -@hyperloglog -@pubsub -@bitmap -@admin -@blocking -@set -@slow +smove +bzpopmax +bitfield_ro +spop +pfadd +smismember +sadd +sismember +srem +bzpopmin +publish +scard +getbit +lastsave' (context: type source line 147 file /Users/matolson/git/redis/tests/unit/acl.tcl cmd {assert_equal "+@all -@$category" $cmdstr} proc ::test)

@madolson
Copy link
Contributor Author

Also worth asking the question if we should just redo this code path, since it's kind of weird that we try to "rewrite" the categories that were given.

@oranagra
Copy link
Member

by "redo" you mean to remember the input so that we can emit it on rewrite without re-composing it?
i don't like that, it means we have redundant info in memory, and a chance for it to conflict with itself.

@madolson
Copy link
Contributor Author

@oranagra I missed your other comment about altering this code. I didn't really have a suggestion for how we could implement it, I was just thinking I don't really like it.

@oranagra
Copy link
Member

@madolson you mean the conversation about the "redo"?
so let's leave it for the future.
i suppose this one can be merged, right?

@madolson madolson merged commit d310beb into redis:unstable Oct 28, 2020
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 9, 2020
This was referenced Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants