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

Dataset not found during frequent writes #2338

Closed
wjones127 opened this issue May 15, 2024 · 0 comments · Fixed by #2396
Closed

Dataset not found during frequent writes #2338

wjones127 opened this issue May 15, 2024 · 0 comments · Fixed by #2396
Assignees
@wjones127
Labels
bug Something isn't working

Comments

@wjones127
Copy link
Contributor

We got a user report where they got Dataset not found randomly. This is happening on AWS NFS. 1 This had the error:

lance error: Dataset at path opt/netapp/wlmai/dbs/lance/chunks.lance was not found: LanceError(IO): Generic LocalFileSystem error: Unable to access metadata for opt/netapp/wlmai/dbs/lance/chunks.lance/_versions/.tmp_37.manifest_bb2eceae-ab02-43b1-8570-e4d1a6667932: IO error for operation on /opt/netapp/wlmai/dbs/lance/chunks.lance/_versions/.tmp_37.manifest_bb2eceae-ab02-43b1-8570-e4d1a6667932: No such file or directory (os error 2), /home/build_user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lance-table-0.10.16/src/io/commit.rs:89:26, /home/build_user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/lance-0.10.16/src/dataset/builder.rs:229:31

It seems to be failing on getting metadata for a temporary manifest file.

I think where this is happening is we are using list_with_delimeter to get the manifest versions.

lance/rust/lance-table/src/io/commit.rs

Lines 89 to 91 in b61850b

let manifest_files = object_store
.list_with_delimiter(Some(&base.child(VERSIONS_DIR)))
.await?;

The implementation of LocalFileSystem::list_with_delimeter calls WalkDir and then various metadata operations on those file references 2. My suspicion is the original WalkDir call sometimes picks up the temporary manifests, but by the time they are calling the metadata operations they have been deleted.

There's two possible solutions to this:

  1. We move the temporary manifests to a different directory, so they aren't part of the WalkDir.
  2. We change the logic to use a pointer to the latest version, and use head to check if there are any newer versions. This is what I lean towards, since it will also help with perf bug: Inserting data is O(num versions) #2318. See also Replace _lastest.manifest and change manifest naming scheme #1362.

Footnotes

  1. https://discord.com/channels/1030247538198061086/1237660577749274684/1239179004364456039 ↩

  2. https://github.com/apache/arrow-rs/blob/d17b2067d637c563eb315571e719807a6f482784/object_store/src/local.rs#L554-L567 ↩
@wjones127 wjones127 added the bug Something isn't working label May 15, 2024
@wjones127 wjones127 self-assigned this May 15, 2024
@wjones127 wjones127 mentioned this issue May 26, 2024
Merged
3 tasks
wjones127 added a commit that referenced this issue May 29, 2024
@wjones127 @westonpace
perf: optimize IO path for reading manifest (#2396)
9a0fa1f 
Fixes #2338
Partially addresses #2318

**For a dataset on local SSD with 8,000 versions, we get 6x faster load
time and 3x faster append.**

* Added special code path for local filesystem for finding latest
manifest. This path skips the `metadata` call for paths that aren't
relevant, both fixing #2338 and improving performance on local
filesystems overall.
* Fixed code path where we were reading the manifest file twice
* Changed `CloudObjectReader` and `LocalFileReader` to both cache the
file size, so we aren't making multiple calls to get the size of the
same object/file. Also allowed passing the size when opening, in case we
already have it from a list operation.
* Deprecated some more methods for loading a dataset, in favor of using
`DatasetBuilder`. Also consolidated the implementations to use
`DatasetBuilder`, so we have fewer code paths to worry about and test.

## TODO

* [x] Cleanup
* [x] Add IO unit test for loading a dataset
* [x] Check repro from 2318

---------

Co-authored-by: Weston Pace <weston.pace@gmail.com>
eddyxu pushed a commit that referenced this issue May 29, 2024
@wjones127 @westonpace @eddyxu
perf: optimize IO path for reading manifest (#2396)
4980ebc 
Fixes #2338
Partially addresses #2318

**For a dataset on local SSD with 8,000 versions, we get 6x faster load
time and 3x faster append.**

* Added special code path for local filesystem for finding latest
manifest. This path skips the `metadata` call for paths that aren't
relevant, both fixing #2338 and improving performance on local
filesystems overall.
* Fixed code path where we were reading the manifest file twice
* Changed `CloudObjectReader` and `LocalFileReader` to both cache the
file size, so we aren't making multiple calls to get the size of the
same object/file. Also allowed passing the size when opening, in case we
already have it from a list operation.
* Deprecated some more methods for loading a dataset, in favor of using
`DatasetBuilder`. Also consolidated the implementations to use
`DatasetBuilder`, so we have fewer code paths to worry about and test.

## TODO

* [x] Cleanup
* [x] Add IO unit test for loading a dataset
* [x] Check repro from 2318

---------

Co-authored-by: Weston Pace <weston.pace@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@wjones127 wjones127

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

perf: optimize IO path for reading manifest wjones127/lance
1 participant
@wjones127