Decouple the code in the fragment directory from StorageManager.
#4453
Conversation
…to `FragmentInfo` and `FragmentMetadata`.
|
This pull request has been linked to Shortcut Story #23374: Move StorageManager::load_fragments to Array. |
teo-tsirpanis
force-pushed
the
fragment-loading-sm
branch
from
9bcc45d to
8e4f1ab
Compare
We cannot capture structured binding variables in lambdas.
There was a problem hiding this comment.
From my reading it seems like you've made the following changes:
| StorageManager Method | Destination |
|---|---|
| load_array_schemas_and_fragment_metadata | FragmentInfo::load_array_schemas_and_fragment_metadata |
| array_load_fragments | Inlined into Array::load_fragments |
| load_fragment_metadata | FragmentMetadata::load |
| load_consolidated_fragment_meta | Free function in fragment_info.cc |
| load_filtered_fragment_uris | Inlined into FragmenInfo::load_array_schemas_and_fragment_metadata |
The only thing I'm questioning there is whether load_array_schemas_and_fragment_metadata belongs in FragmentInfo or ArrayDirectory. It seems a bit awkward dropping it into FragmentInfo but it also only feels marginally better if we were to drop that into ArrayDirectory instead.
Maybe @KiterLuc can point us where he think that'd fit better?
Personally I think having all storage access flow through array directory would be best, but looking around that seems like it'll be a long term project once we've finished removing StorageManager.
Beyond that, the only thing that was a definite change was to use resources_ as the member variable name consistently.
All of the other work appears to be the obvious updates that I would expect so other than these two minor questions/changes I'll be ready to +1
teo-tsirpanis
force-pushed
the
fragment-loading-sm
branch
from
cf4a902 to
038a4a4
Compare
Hmm, at present |
| resources.stats().start_timer("sm_read_load_consolidated_frag_meta"); | ||
|
|
||
| // No consolidated fragment metadata file | ||
| if (uri.to_string().empty()) |
There was a problem hiding this comment.
We can change this to throw and make the return values not optional. The only place it is used dereference the returned tile, which will throw anyways.
tiledb/sm/fragment/fragment_info.cc
Outdated
| std::vector<shared_ptr<Tile>> fragment_metadata_tiles(meta_uris.size()); | ||
| std::vector<std::vector<std::pair<std::string, uint64_t>>> offsets_vectors( | ||
| meta_uris.size()); | ||
| auto status = |
There was a problem hiding this comment.
Move throw_if_not_ok on this line and remove the status.
tiledb/sm/fragment/fragment_info.h
Outdated
| * @param memory_tracker The memory tracker of the array | ||
| * for which the fragment metadata is loaded. | ||
| * @param enc_key The encryption key to use. | ||
| * @return tuple of Status, latest ArraySchema, map of all array schemas and |
SC-23374
The
StorageManagermethods that load fragment info and metadata were moved to theFragmentInfoandFragmentMetadataclasses. These classes also useContextResourcesinstead ofStorageManager.I was going to make a
fragmentobject library but it also depends onrestwhich is not yet componentized.TYPE: IMPROVEMENT
DESC: Refactor the fragment info loading code to reduce coupling.