-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: Implemented synced enforcer #83
Conversation
Signed-off-by: Yash Pandey (YP) <[email protected]>
Signed-off-by: Yash Pandey (YP) <[email protected]>
Signed-off-by: Yash Pandey (YP) <[email protected]>
Signed-off-by: Yash Pandey (YP) <[email protected]>
Signed-off-by: Yash Pandey (YP) <[email protected]>
884e4d6
to
4ebf55e
Compare
Signed-off-by: Yash Pandey (YP) <[email protected]>
@bokket feel free to commit on this branch. I am sure there's a lot of room for improvement here. |
Signed-off-by: EmperorYP7 <[email protected]>
Signed-off-by: EmperorYP7 <[email protected]>
Signed-off-by: EmperorYP7 <[email protected]>
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.
So, SetWatcher()
will be invoked whenever there's a change in the database(policy)?
// SetWatcher sets the current watcher. | ||
void SyncedEnforcer ::SetWatcher(shared_ptr<Watcher> w){ | ||
watcher = w; | ||
return watcher->SetUpdateCallback(&SyncedEnforcer::UpdateWrapper); |
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.
@xcaptain I think this would be of some context to #38 (comment).
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.
@EmperorYP7 Nice, and I think you can also add SetWatcher
to enforcer
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.
Sure thing!
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.
LGTM
Plz provide smaller PRs. |
🎉 This issue has been resolved in version 1.12.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Signed-off-by: Yash Pandey (YP) [email protected]
This PR fixes #38
Most of the functions of SyncedEnforcer were straight-forward to implement but the
AutoLoadPolicy
needed a workaround since it utilized GoLang's ticker from the "time" module and concurrency. For this, I created a custom concurrent ticker that takes in the duration and afunction<void()>
for a callback. Then passed in the following to callback of ticker which is called at every tick:Note that
n
is anatomic_int
which is thread-safe.When
StopAutoLoad()
is called, the ticker stops all worker threads deployed by the ticker. However, this won't destroy the instance of the ticker. I also created test files to verify everything is implemented correctly. (The test policy won't change in real-time during a test)Overheads involved:
Apart from the futures obtained from ticker threads, ticker also collects futures from callback at each tick since they're called asynchronously as well. This might lead to a pile of useless data given enough uptime of
AutoLoadPolicy()
and scope ofSyncedEnforcer
.Tasks remaining
BatchEnforcer
,BatchWithMatcher
yet to be implemented inEnforcer
.UpdatePolicy
,UpdateNamedPolicy
,UpdatePolicies
andUpdateNamedPolicies
yet to be implemented inEnforcer
.UpdateGroupingPolicy
,UpdateNamedGroupingPolicy
yet to be implemented inEnforcer
.I'll implement this in another PR 'cause this is enormous already.