Dataset usage tracking and purging of expired temporary datasets #503
Conversation
Adding convenience method to DatasetManager to simplify creating temporary datasets.
Adding mock implementation and unit tests class for DatasetManager and the methods it implements directly, along with set of tests for the create_temporary method.
Implementing the logic to initiate, hold, and release linkage between DatasetManager and DatasetUser.
Updating deserialization process for Dataset instances to drop microseconds from applicable datetime attributes, via a Pydantic validator.
Updating service manager for dataservice with functions to support purging expired temporary datasets.
Adding async task to service manager setup in main dataservice function to manage temporary datasets.
robertbartel
force-pushed
the
f/temp_ds_purge/main
branch
from
0bd50ae
to
4e0e97f
Compare
| @@ -671,6 +686,41 @@ def filter(self, base_dataset: Dataset, restrictions: List[Union[ContinuousRestr | |||
| """ | |||
| pass | |||
|
|
|||
| def get_dataset_user(self, uuid: UUID) -> DatasetUser: | |||
There was a problem hiding this comment.
Any reason not to return Optional[DatasetUser] here instead of throwing?
There was a problem hiding this comment.
The intent for the function is that it will only receive a user UUID value corresponding to a user instance that the manager is linked with (perhaps the caller even received the UUID from the manager; that's done in the ServiceManager in one spot). If some other UUID is given, it's fundamentally a problem.
I'd also expect real-world calls without a proper corresponding user object to generally be bug related: e.g., the manager failed to completely track a user properly when it was linked. An immediate error made more sense to me there.
| next invocation). | ||
| """ | ||
| # Get these upfront, since no meaningful changes can happen during the course of an iteration | ||
| temp_datasets = {ds_name: ds for ds_name, ds in self.get_known_datasets().values() if ds.is_temporary} |
There was a problem hiding this comment.
I think this is fine so long as temp datasets can only be used by a single job. If a temp dataset can be referenced by multiple jobs, I think there is a data race if a job starts that uses a temp dataset but the dataservice is waiting to check for new jobs. The temp dataset that job depends on could be swept.
There was a problem hiding this comment.
Good catch. I'll need to put something in ServiceManager.perform_checks_for_job to block the mark and sweep routine (or maybe even just the sweep portion). I'll also either have to unmark in this function, or perhaps move the logic for linking job users to here.
There was a problem hiding this comment.
I have made some changes to account for this.
Co-authored-by: Austin Raney <[email protected]>
Updating dataservice ServiceManager class to block processing of managing temporary datasets at all times when datasets may be marked as fulfilling data requirements, including updating the latter to also do linking to dataset users at the same time.
Ensuring that dataservice service.py uses items() function rather than values().
|
@aaraney, this is ready for re-review, although I am a little concerned about the test fails. I was having the same failures after my initial requested changes, but I cleared them up locally by switching |
|
@robertbartel, yeah that is odd about the tests failing. I pulled your branch and the tests passed fine for me. |
|
@robertbartel, like you suspected, after clearing the runner caches the tests are passing again. |
Adds functionality to maintain links between datasets and things using the datasets (e.g., a job), in order to ensure that the life of temporary datasets is extended based on their use.
Also adds service functionality to periodically purge temporary datasets that have expired.