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

feat!: discoveryId instead of key for entries #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gmaclennan
Copy link
Member

@gmaclennan gmaclennan commented Nov 27, 2023

This is stacked on #24, but we should merge and publish that first, then merge and publish this as a separate version, because the breaking changes will require changes to mapeo-core-next.

BREAKING CHANGE: this changes the type of Entry to have the core
discoveryId (the discovery key as a hex string) instead of the core key.

Since our mapeo-core code changed to using the discovery key for version
ids, we need to calculate the discovery key for each entry, which has a
performance cost (it's a hash operation). This change avoids the need
for any hashing when processing entries.

@gmaclennan gmaclennan self-assigned this Nov 27, 2023
@gmaclennan gmaclennan requested a review from achou11 November 27, 2023 03:40
Base automatically changed from feat/non-ready-cores to feat/initial-indexing-state November 28, 2023 12:36
Base automatically changed from feat/initial-indexing-state to main November 28, 2023 12:41
@achou11
Copy link
Member

achou11 commented Dec 5, 2023

can you rebase this PR so that it's updated with main? Think Github is including the commits from the PR that this was stacked on, so the diff is much larger 😅

guessing the relevant changes are in 905e2fc, so can review that for now

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

(relevant) changes lgtm! just need to fix the rebasing stuff so won't officially approve for now

@gmaclennan gmaclennan force-pushed the feat/entry-discovery-id branch from 905e2fc to 3ccc387 Compare December 6, 2023 03:13
@gmaclennan
Copy link
Member Author

ok now rebased. I'm going to hold off merging in case we need to make any other bug fixes to this module, because I haven't modified mapeo-core to handle this breaking change yet and we don't want to become blocked by that.

BREAKING CHANGE: this changes the type of `Entry` to have the core
discoveryId (the discovery key as a hex string) instead of the core key.

Since our mapeo-core code changed to using the discovery key for version
ids, we need to calculate the discovery key for each entry, which has a
performance cost (it's a hash operation). This change avoids the need
for any hashing when processing entries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants