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

Remove Status_StorageManagerError and Migrate group_metadata_vacuum out of StorageManager. #5034

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

bekadavis9
Copy link
Contributor

@bekadavis9 bekadavis9 commented May 31, 2024

Remove Status_StorageManagerError and its occurrences.
Related subsequent fixes:

  • Remove Logger::status_no_return_value and replace occurrences with Logger::error.
  • Remove Status and std::optional from returned tuple of Array::open_for_writes.
  • de-Status Context::init_loggers.
  • Migrate StorageManagerCanonical::group_metadata_vacuum -> static Group::vacuum_metadata.

[sc-48630]
[sc-48639]


TYPE: NO_HISTORY
DESC: Remove Status_StorageManagerError and Migrate group_metadata_vacuum out of StorageManager.

@bekadavis9 bekadavis9 changed the title Remove Status_StorageManagerError. Remove Status_StorageManagerError and Migrate group_metadata_vacuum out of StorageManager. May 31, 2024
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.

One change for which exception to throw needed.

Also, we should make stories for removal of StorageManager for (1) class Array and (2) Consolidator::create.

throw GroupException("Cannot vacuum group metadata; Group does not exist");
}

StorageManager storage_manager(resources, resources.logger(), config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needing to make a new storage manager just to call Consolidator::create is quite a red flag. The problem isn't in this code narrowly, though; it's the implementation of the consolidator.

We should target changing Consolidator::create for a refactor. It should be possible to convert it to ContextResources and RestClient, but it will also entail changing a constructor signature for Array. It's definitely going to require a concerted effort. Removing all the array and group functions from StorageManager should happen first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already a followup story to remove usage of StorageManager from the consolidation code: https://app.shortcut.com/tiledb-inc/story/47947/eliminate-storagemanager-arguments-in-consolidation-code-in-favor-of-contextresources

tiledb/sm/storage_manager/context.cc Outdated 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.

LGTM

@bekadavis9 bekadavis9 force-pushed the rd/removals-status_storage_manager_error branch from 1e06ea7 to 2b96634 Compare June 5, 2024 19:13
@bekadavis9 bekadavis9 force-pushed the rd/removals-status_storage_manager_error branch from 0267a8c to b402392 Compare June 6, 2024 14:15
@KiterLuc
Copy link
Contributor

KiterLuc commented Jun 7, 2024

Merging with the unit URI failures as they are unrelated. [sc-https://app.shortcut.com/tiledb-inc/story/48975/intermittent-segfaults-in-unit-uri-on-windows]

@KiterLuc KiterLuc merged commit 1326ed4 into dev Jun 7, 2024
56 of 60 checks passed
@KiterLuc KiterLuc deleted the rd/removals-status_storage_manager_error branch June 7, 2024 08:23
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.

3 participants