Skip to content
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

MLG-1.4.0: Remove support for SquashedPatch Edits (fixes #18) #19

Merged
merged 16 commits into from
Jul 16, 2020

Conversation

OstlerDev
Copy link
Collaborator

@OstlerDev OstlerDev commented Sep 16, 2019

Changelog

Added

  • Added support for RFC6902 JSON Patches in OIP042 Edits
  • Added EditSyncComplete and MultipartSyncComplete bool flags to API response of /oip/sync/status
  • Multiparts are now be marked as stale if they are "broken" and old
  • Edits are now be marked as Invalid if they are broken/defective

Removed

  • Removed support for Squashed JSON Patches in OIP042 Edits

@OstlerDev OstlerDev added the bug Something isn't working label Sep 16, 2019
@OstlerDev OstlerDev self-assigned this Sep 16, 2019
Copy link
Member

@bitspill bitspill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the comments I've left within the PR I have a few overall comments

  1. Would Invalid be a more fitting term than Defective for an edit that can't be applied?

  2. As far as I can tell the system doesn't account for ordering of edits beyond sorting by timestamp which only works if edits are each in different blocks. Two (or more) edits in the same block would be assigned the same timestamp and thus ordering would be non-deterministic perhaps a sequence number should be added such that before applying an edit it's verified that this is indeed the proper next edit

  3. Processing edits should wait until after multiparts have completed otherwise the majority of edits will all complain about being unable to find their record by using the following instead of IsInitialSync

if !oipSync.MultipartSyncComplete {
	return
}

modules/oip042/edit.go Outdated Show resolved Hide resolved
modules/oip042/edit.go Outdated Show resolved Hide resolved
modules/oip042/edit.go Outdated Show resolved Hide resolved
modules/oip042/edit.go Outdated Show resolved Hide resolved
modules/oip042/edit.go Outdated Show resolved Hide resolved
datastore/mappings/oip042_edit.json Outdated Show resolved Hide resolved

// If we are not still syncing for the first time, and the remaining count is exactly the same as last time we checked,
// then it is a good indicator that these Multiparts are stale
if (!wasInitialSync && previousMultipartCount == len(multiparts) && previousMultipartCount < 10000) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this statement remains false if the stale count exceeds 10,000

@OstlerDev
Copy link
Collaborator Author

After long ado, here are the changes you requested.

  1. Invalid is a better term than Defective, I have gone ahead and changed that to be Invalid.

  2. Edits are being sorted by the timestamp included inside of the edit (edit.timestamp) instead of using the block time that gets assigned into meta.timestamp. As long as the users set time timestamp time inside the edit, they should be processed in order even if they were included into the same block.

  3. I have gone ahead and changed it from using isInitialSync to use MultipartSyncComplete

  4. I have pushed up a commit that addresses the bulk of the other changes you opened discussions about.

As for the issue with the multipart stale count, I am not sure of the best way to solve this issue, but I agree that we could have a lockup if there are >= 10,000 stale multiparts.

Please let me know if there are any other changes you are looking for.

@OstlerDev OstlerDev requested a review from bitspill July 15, 2020 16:53
@OstlerDev OstlerDev changed the base branch from master to oip5 July 15, 2020 16:55
@OstlerDev OstlerDev changed the base branch from oip5 to master July 15, 2020 16:56
@bitspill bitspill merged commit bdba243 into oipwg:master Jul 16, 2020
OstlerDev added a commit to mediciland/oip that referenced this pull request Jul 16, 2020
@OstlerDev OstlerDev deleted the mlg-1.4.0 branch July 16, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants