Updates from permit internal fork #546
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for opal-docs canceled.
|
roekatz
force-pushed
the
permit/changes-from-internal-fork
branch
from
d2fa7ef
to
7e9b778
Compare
roekatz
force-pushed
the
permit/changes-from-internal-fork
branch
9 times, most recently
from
5487f13
to
04cf733
Compare
roekatz
force-pushed
the
permit/changes-from-internal-fork
branch
from
04cf733
to
475c36c
Compare
asafc
reviewed
Mar 3, 2024
There was a problem hiding this comment.
@roekatz i reviewed the changes and they are overall ok - however - i did a comparison to the private fork and there are a bunch of changes that require deeper review and justification.
let's sync on that when you are available.
I left detailed comments in this ticket:
https://linear.app/permit/issue/PER-9276/opal-public-candidate-to-opal-private-cr-fixes
The same process does all the fetches, the file lock mechanism isn't fair (so a task can wait a lot of time before entering the lock)
This should utilize pygit's memory caching and improve performance
WARN for unhealthy, DEBUG for healthy
…tch (of the relevant repo)
For example - due to broadcaster disconnection
The introduction of shards created a bug, because we might skip force fetching a url already fetched for another scope, when the scopes are not under the same shard (aka local clone)
The temp file is used to achieve atomic replacement of the older backup (using os.rename). But when using a fixed name for the temp file, different replicas sharing the volume could partially override it before the rename; thus making it possible to end up with a partial backup file
Should create different local branches to track the last reported state for each scope separated
Fix the local branch name (that tracks the remote branch for each scope) to f"scopes/{scope_id}".
When the format f"{branch}/scope_{scope_id}" was used, the branch name would be invalid when `branch == "master"`.
(can't create branch "master/..." if "master" already exists).
Should check if f"scopes/{self._scope_id}" is valid reference name, otherwise it always fails and we always encode it as hex
This causes a race when using the last stored bundle hash to fetch policy from server, Eventually client could try to store the same bundle twice (current hash is updated after the update is stored, but is checked before it's fetched)
Queue.get raised an error because queue creationg and queue handling (.get) occurred on different tasks
roekatz
force-pushed
the
permit/changes-from-internal-fork
branch
from
93c49ab
to
76fd0bf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.