-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Dgraph): Subscribe to ACL updates instead of polling. #6459
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks fine to me but did you test the changes? Can you add a test for it?
Change some ACLs and then run a query?
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @martinmr, and @vvbalaji-dgraph)
edgraph/access_ee.go, line 322 at r1 (raw file):
var maxRefreshTs uint64 retrieveAcls := func(refreshTs uint64) error { if refreshTs <= maxRefreshTs {
Nice.
edgraph/access_ee.go, line 350 at r1 (raw file):
closer.AddRunning(1) go worker.SubscribeForUpdates(aclPrefixes, func(kvs *bpb.KVList) { if kvs == nil || len(kvs.Kv) == 0 {
Is this even possible? The subscription should be called only when there's data to send.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, and @vvbalaji-dgraph)
edgraph/access_ee.go, line 350 at r1 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
Is this even possible? The subscription should be called only when there's data to send.
I don't think so but I added it as a sanity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a lot of acl test in the acl_test.go file using time.sleep. Can you remove all the time.sleep calls from the tests? The tests should without time.sleep after this PR.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it but there are tests are flaky without any delays. Makes sense to me since the query is not instantaneous. Also, we need to wait for all the alphas to get the update.
I reduced the delay to 5 seconds. Most tests passed if I made the delay smaller but there were a few that were flaky with a value like 3 seconds. Keeping it to five to err on the side of caution.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, and @vvbalaji-dgraph)
This change is
Docs Preview: