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

Migrate delete_array and delete_fragments from StorageManager to Array. #4880

Merged
merged 3 commits into from May 15, 2024

Conversation

bekadavis9
Copy link
Contributor

@bekadavis9 bekadavis9 commented Apr 15, 2024

Migrate Array APIs out of StorageManager: delete_array and delete_fragments. Note: this migration required addition of new, static versions of these internal APIs on class Array, which effectively do exactly what the versions in StorageManager were previously doing. This allows for a "hierarchical-like" structure among the inter-dependent APIs delete_group, delete_array, and delete_fragments.


[sc-23372]
[sc-47161]
[sc-47162]


TYPE: IMPROVEMENT
DESC: Migrate Array APIs out of StorageManager: delete_array and delete_fragments.

@ypatia
Copy link
Contributor

ypatia commented Apr 16, 2024

FYI, the REST-CI failure should go away if you rebase to latest dev , it's due to sc-44928

@bekadavis9 bekadavis9 force-pushed the rd/sm-array-migrations-delete_fragments branch from c630b41 to 5db589a Compare April 16, 2024 13:53
@bekadavis9 bekadavis9 force-pushed the rd/sm-array-migrations-delete_fragments branch from 5db589a to 373ca5b Compare May 1, 2024 20:47
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

This PR highlights a pre-existing problem with the way deletion is handled, but makes the problem worse. The symptom of the problem is the overloading of the names delete_array and delete_fragments. There are four functions in total; each name is both a member function and a static function. There are two aspects to the problem:

  1. Insofar as underlying behavior goes, there's no good clarity to what the underlying operation is, how the remote operation is handled, and dispatch between the two. Those changes are beyond the scope of this PR. Such changes would require a rewrite.
  2. Insofar as understanding the behavior goes, it's quite confusing as to what functions do what etc. This is primarily a documentation problem and is definitely within the scope of this PR.

We need better documentation for each of the four delete_* functions affected. Three are new; one was already present (member delete_fragments). Each of the functions needs to describe what it does in what circumstances and distinguish itself from its partner.

In the long term we need a coherently design for these capabilities; that time is not during this PR. Yet we will need to write such a thing in the future, and we need better documentation now so that future effort will have at least a signpost about what direction to head.

tiledb/sm/array/array.cc Outdated Show resolved Hide resolved
tiledb/sm/array/array.h Show resolved Hide resolved
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

The documentation is still inadequate to address the basic confusion I had in first reviewing this PR.

  • Why are there both static and member functions with the same name?
  • What's the difference?
  • When to use one instead of the other?

I'm not sure it's within the scope of this PR to find good answers for these, but at the very least we need to acknowledge the confusion.

Looking at the actual code, this much is clear. None of these functions seem to have the right signature. There's code in the C API interface functions that belongs in these functions, and vice-versa.

At minimum, we should do all these:

  • Mark these functions inside of @section Maturity as interim versions awaiting a rewrite.
  • Pairs of functions should reference each other.
  • Mention that the static functions are a direct legacy of StorageManager.

Please do not confine the documentation to the bare minimum.

@bekadavis9 bekadavis9 force-pushed the rd/sm-array-migrations-delete_fragments branch from ea8f2ee to 4d0edf1 Compare May 13, 2024 15:47
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

LGTM

@KiterLuc KiterLuc merged commit 85f8388 into dev May 15, 2024
59 checks passed
@KiterLuc KiterLuc deleted the rd/sm-array-migrations-delete_fragments branch May 15, 2024 00:38
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.

None yet

4 participants