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.

Sign up for GitHub

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

Jump to bottom

implement ExternalManifestCommitHandler and trait ExternalManifestStore #1190

Merged
merged 1 commit into from
Sep 1, 2023
Merged

implement ExternalManifestCommitHandler and trait ExternalManifestStore #1190

merged 1 commit into from
Sep 1, 2023

Conversation

chebbyChefNEQ
Copy link
Contributor

@chebbyChefNEQ chebbyChefNEQ commented Aug 31, 2023 •
edited
Loading

part of #1183 <-- detailed design is here too

This PR implements

  • trait ExternalManifestStore which abstracts an KV-like store with put_if_not_exists capability
  • ExternalManifestCommitHandler which takes a trait object of ExternalManifestStore and handles concurrent write to lance dataset using this dyn ExternalManifestStore

TODO:

  • complete documentation
  • add unit testing

@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch 7 times, most recently from 345ab02 to 85bf65a Compare August 31, 2023 14:18
wjones127
wjones127 requested changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

This looks like it will work, but I have a few questions.

rust/src/io/external_manifest.rs Outdated Show resolved Hide resolved
rust/src/dataset.rs Outdated Show resolved Hide resolved
rust/src/io/commit.rs Outdated Show resolved Hide resolved
rust/src/io/external_manifest/commit.rs Outdated Show resolved Hide resolved
@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch 4 times, most recently from bd9150a to bfe46f9 Compare August 31, 2023 17:33
eddyxu
eddyxu reviewed Aug 31, 2023
View reviewed changes
rust/src/dataset.rs Outdated Show resolved Hide resolved
wjones127
wjones127 approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

There's some cleanup we can do as a follow up, but the core logic looks sound.

westonpace
westonpace reviewed Aug 31, 2023
View reviewed changes
Copy link
Contributor

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

A few questions, this is quite complex so I might've gotten lost myself :)

rust/src/io/external_manifest/commit.rs Outdated Show resolved Hide resolved
rust/src/io/external_manifest/commit.rs Outdated Show resolved Hide resolved
rust/src/io/commit.rs Outdated Show resolved Hide resolved
rust/src/io/external_manifest/commit.rs Outdated Show resolved Hide resolved
rust/src/io/external_manifest/commit.rs Outdated Show resolved Hide resolved
rust/src/io/external_manifest/commit.rs Outdated Show resolved Hide resolved
rust/src/dataset.rs Outdated Show resolved Hide resolved
@chebbyChefNEQ chebbyChefNEQ mentioned this pull request Sep 1, 2023
Merged
@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch from bfe46f9 to 57c6a00 Compare September 1, 2023 19:12
@chebbyChefNEQ chebbyChefNEQ mentioned this pull request Sep 1, 2023
Closed
@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch 6 times, most recently from 1df378a to da679c5 Compare September 1, 2023 19:41
@chebbyChefNEQ chebbyChefNEQ mentioned this pull request Sep 1, 2023
Open
5 tasks
@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch from da679c5 to 5de2e97 Compare September 1, 2023 20:04
@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch from 5de2e97 to bc2b036 Compare September 1, 2023 20:07
@chebbyChefNEQ chebbyChefNEQ force-pushed the rmeng/external-manifest branch from bc2b036 to 330ec4b Compare September 1, 2023 20:18
@chebbyChefNEQ chebbyChefNEQ merged commit 14807bc into main Sep 1, 2023
13 checks passed
@chebbyChefNEQ chebbyChefNEQ deleted the rmeng/external-manifest branch September 1, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonpace westonpace westonpace left review comments

@eddyxu eddyxu eddyxu left review comments

@wjones127 wjones127 wjones127 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chebbyChefNEQ @eddyxu @westonpace @wjones127