-
Notifications
You must be signed in to change notification settings - Fork 57
Checkpoints introduction for version changes #520
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
base: dev-r85-v2-pull
Are you sure you want to change the base?
Conversation
- introduce get_delta method with caching - introduce delta contrller
- added tests for pull and checkpoints
- accept numbers in endpoint - consider to store diff instead of diffs in db
Pull Request Test Coverage Report for Build 18385680927Details
💛 - Coveralls |
server/mergin/sync/models.py
Outdated
PushChangeType.UPDATE, | ||
PushChangeType.UPDATE_DIFF, | ||
): | ||
# create + update = create with updated info |
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.
yes, for client this is completely new file so any subsequent updates will appear as a new file to client, just with the most recent metadata - pleas update a comment
current.change = existing.change | ||
current.diffs = [] | ||
else: | ||
result[path] = current |
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.
ideally this should never happen? - create + create
server/mergin/sync/models.py
Outdated
current.diffs.extend(existing.diffs or []) | ||
result[path] = current | ||
else: | ||
# delete + anything = anything |
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 don't think so - after delete we should only have a create action, otherwise we have weird inconsistent history - I'd be more specific here.
server/mergin/sync/models.py
Outdated
elif existing.change == PushChangeType.UPDATE_DIFF: | ||
if current.change == PushChangeType.UPDATE_DIFF: | ||
# update_diff + update_diff = update_diff with latest info | ||
current.diffs.extend(existing.diffs or []) |
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.
should not we extend previous.diffs with current diff(s)?
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.
?
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.
renamed to previous
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 comment was about making sure that order of diffs in list is correct
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.
let's upgrade diffs existing first, current last
- rename table to project version delta with column changes - rename classes - add get_delta_changes to project instance (nice) - fix migration to int
We are introducing new table for storing chckpoints (project_version_change), where rank and delta json for current version is stored. If version are in logarithms logic with base=4, then store higher level checkpoints if needed.
Structure for delta json: List[DeltaData]. If there is not diff, diff key is not provided in result.
Delta creation logic
Migration
Migrate file history and file diffs to merged delta in project_version_change table.