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

Add MemFS context isolation test, and use memfs in quickstart dense example. #3438

Merged
merged 5 commits into from
Apr 12, 2023

Conversation

bekadavis9
Copy link
Contributor

Through debug, we've discovered that the MemFS was not getting initialized in the VFS, despite being "enabled" by default. A global Context and VFS are typically used on the object so this bug was previously overlooked. This work item initializes the MemFS in the VFS and adds an example test, which also highlights the need for a global Context on MemFS.


TYPE: BUG
DESC: Initialize MemFS in the VFS

@bekadavis9
Copy link
Contributor Author

[sc-19989]

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #19989: memfs initialization and testing.

Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Works great from R where we have a single Context object by default / design (allowing more or others as needed, but typically users just want one) and for that the (current ?) memfs setup works just fine.

@ihnorton ihnorton requested a review from shaunrd0 August 10, 2022 01:59
ypatia
ypatia previously requested changes Aug 10, 2022
Copy link
Member

@ypatia ypatia left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, good catch! I am worried about introducing a 2-step initialization and shared pointers, could you please check if we can avoid that before merging?

tiledb/sm/filesystem/vfs.cc Outdated Show resolved Hide resolved
tiledb/sm/filesystem/mem_filesystem.h Outdated Show resolved Hide resolved
examples/cpp_api/memfs.cc Outdated Show resolved Hide resolved
Copy link
Member

@ypatia ypatia left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! Let's wait for Isiah's/Eric's opinion on the ownership issue before we can proceed.
Also could you please add tests for your changes, at least a regression test for the bug you are fixing? I think it will be almost identical to your example, but I am not sure examples are run on CI so a test would make sense here.

Copy link
Contributor

@shaunrd0 shaunrd0 left a comment

Choose a reason for hiding this comment

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

Was able to reproduce VFS.RemoveDir() errors using C# API and after building / linking with this branch errors were resolved. Thanks, this was good to review - lots of good information here

@eddelbuettel
Copy link
Contributor

This is a bit confusing:

edd@rob:~/git/tiledb/examples/cpp_api(rd/memfs_error_debug)$ ./memfs 
Error: Must use process global Context on memfs.
2 3 4 6 7 8 
The MemFilesystem is associated with a single VFS instance.
edd@rob:~/git/tiledb/examples/cpp_api(rd/memfs_error_debug)$ 

So it throws on the attempt to vfs.remove_dir() the one memfs that there is, in the one context. If we disallow that, why not simply error on entry in the remove_array() (demo) function?

@ihnorton ihnorton marked this pull request as draft August 16, 2022 04:18
@bekadavis9 bekadavis9 marked this pull request as ready for review April 12, 2023 15:57
@bekadavis9 bekadavis9 force-pushed the rd/memfs_error_debug branch from 759d989 to 79e16d1 Compare April 12, 2023 16:33
@bekadavis9 bekadavis9 force-pushed the rd/memfs_error_debug branch from 79e16d1 to cc66664 Compare April 12, 2023 17:15
@ihnorton ihnorton changed the title Initialize MemFS in VFS and add example Add MemFS context isolation test, and use memfs in quickstart dense example. Apr 12, 2023
@ihnorton ihnorton dismissed ypatia’s stale review April 12, 2023 20:22

No functional changes remaining in this PR.

@ihnorton ihnorton merged commit afebd94 into dev Apr 12, 2023
@ihnorton ihnorton deleted the rd/memfs_error_debug branch April 12, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants