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: Add Defra-Lens support for branching schema #2421

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #2293

Description

Adds Defra-Lens support for branching schema.

Defra will now be able to handle migrations for a branching schema version DAG.

@AndrewSisley AndrewSisley added feature New feature or request area/schema Related to the schema system labels Mar 18, 2024
@AndrewSisley AndrewSisley added this to the DefraDB v0.11 milestone Mar 18, 2024
@AndrewSisley AndrewSisley requested a review from a team March 18, 2024 17:24
@AndrewSisley AndrewSisley self-assigned this Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.86%. Comparing base (d7931ff) to head (07c3167).

❗ Current head 07c3167 differs from pull request most recent head 915761b. Consider uploading reports for the commit 915761b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2421      +/-   ##
===========================================
- Coverage    74.92%   74.86%   -0.06%     
===========================================
  Files          268      268              
  Lines        25888    25875      -13     
===========================================
- Hits         19395    19371      -24     
- Misses        5169     5178       +9     
- Partials      1324     1326       +2     
Flag Coverage Δ
all-tests 74.86% <100.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
lens/history.go 92.86% <100.00%> (+5.23%) ⬆️
lens/lens.go 73.74% <100.00%> (ø)

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7931ff...915761b. Read the comment docs.

lens/history.go Outdated
}
targetHistoryItem, ok := history[targetSchemaVersionID]
if !ok {
// If the target schema version is unknown then there are possible no migrations
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: possibly

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 19, 2024

Choose a reason for hiding this comment

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

cheers, is actually in the wrong place, should be on the right side of no

  • Move word

cols, err := description.GetCollectionsBySchemaRoot(ctx, txn, schemaRoot)
if err != nil {
return nil, err
}

history := map[collectionID]*schemaHistoryLink{}
history := map[schemaVersionID]*schemaHistoryLink{}
schemaVersionsByColID := map[uint32]schemaVersionID{}

for _, c := range cols {
col := c
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: (I know it's from a different PR but I might of missed it then) I don't think this necessary. Why did you decide to do this?

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 19, 2024

Choose a reason for hiding this comment

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

I have no idea, might have made sense at one point, or it might have been a brain-fart.

I'll remove it shortly before merge.

  • Remove odd var copy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found out, it is because we want a pointer to it, and the way Go works means that without the copy col would always be the last col in cols.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for reminding me of this. Could you add a TODO remove when Go 1.22 comment above it please.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 20, 2024

Choose a reason for hiding this comment

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

Ah nice, glad that will fix it - will do

  • Add too comment

@AndrewSisley AndrewSisley force-pushed the 2293-schema-branch-migrations branch 3 times, most recently from 4bc7c0c to d17eceb Compare March 19, 2024 22:24
"github.com/sourcenetwork/defradb/tests/lenses"
)

func TestSchemaMigrationQueryWithP2PReplicatedDocOnOtherSchemaBranch(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: This test should propably be in the net replicator test no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It fits in both directories just as well IMO - I probably prefer it here because I see it more from a schema migration perspective, and you probably see it in the net directory as you see it more from a P2P perspective.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In a sense, everything that is in the net directory also relate to some other test directory. In my mind, as soon as a test is interacting with the P2P system, it would be better placed in the net package.

},
testUtils.WaitForSync{},
testUtils.Request{
// Node 0 should yield results as they were defined, as the newer schema version is
Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 20, 2024

Choose a reason for hiding this comment

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

todo: delete comment

  • delete comment

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for answering all my comments and questions.

@AndrewSisley
Copy link
Contributor Author

LGTM! Thanks for answering all my comments and questions.

Thanks for finding a bunch of stuff (including the ones raised on discord) :)

@AndrewSisley AndrewSisley merged commit 839b505 into sourcenetwork:develop Mar 20, 2024
27 of 28 checks passed
@AndrewSisley AndrewSisley deleted the 2293-schema-branch-migrations branch March 20, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema Related to the schema system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Defra-Lens support for branching schema
2 participants