fix: invalidate metadata cache when dataset fails to load #87

MDunitz · 2021-09-27T17:25:49Z

Reviewers

Functional:

@atolopko-czi

Readability:

@ebezzi

Changes

modify cache code. Update dataset cache to use path to dataset as the cache key (instead of the explorer_url). Invalidate items in the metadata cache when particular errors are raised.
Update tests to use symlinks to allow for dataset cache key change

codecov · 2021-09-27T22:33:16Z

Codecov Report

Merging #87 (26d611b) into main (8e0dd91) will increase coverage by 0.01%.
The diff coverage is 80.00%.

❗ Current head 26d611b differs from pull request most recent head db5f22d. Consider uploading reports for the commit db5f22d to get more accurate results

@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   71.82%   71.83%   +0.01%     
==========================================
  Files         126      126              
  Lines       10086    10106      +20     
==========================================
+ Hits         7244     7260      +16     
- Misses       2842     2846       +4

Flag	Coverage Δ
frontend	`71.83% <80.00%> (+0.01%)`	⬆️
javascript	`71.83% <80.00%> (+0.01%)`	⬆️
smokeTest	`?`
unitTest	`71.83% <80.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/app/app.py	`76.07% <66.66%> (-0.23%)`	⬇️
server/data_common/cache.py	`98.55% <100.00%> (+0.04%)`	⬆️
app/app.py	`76.07% <0.00%> (-0.23%)`	⬇️
data_common/cache.py	`98.55% <0.00%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e0dd91...db5f22d. Read the comment docs.

atolopko-czi · 2021-09-28T13:36:02Z

Changes

modify cache code. Update dataset cache to use path to dataset as the cache key (instead of the explorer_url). Invalidate items in the metadata cache when particular errors are raised.
Update tests to use symlinks to allow for dataset cache key change

So the motivation for this change is to ensure cache provides the latest data if/when an Explorer URL-to-S3 Path mapping is no longer valid for a dataset? When is this occurring in practice?

atolopko-czi

Overall, looks correct! Have Q's and comments, but can approve if you don't find any of them to be a concern.

atolopko-czi · 2021-09-28T13:57:22Z

server/app/app.py

            create_data_function=MatrixDataLoader(
                location=dataset_metadata["s3_uri"], url_dataroot=url_dataroot, app_config=app_config
            ).validate_and_open,
            create_data_args={},
        )


+def expire_metadata_cache(url_dataroot: str = None, dataset: str = None):


suggest evict_dataset_from_metadata_cache, just so it doesn't sound like the entire cache is being expired/cleared.

atolopko-czi · 2021-09-28T13:59:32Z

server/app/app.py

@@ -143,6 +147,7 @@ def wrapped_function(self, dataset=None):
                data_adaptor.set_uri_path(f"{self.url_dataroot}/{dataset}")
                return func(self, data_adaptor)
        except (DatasetAccessError, DatasetNotFoundError, DatasetMetadataError) as e:
+            expire_metadata_cache(self.url_dataroot, dataset)


can call this itself raise an exception that should be handled? or is it reasonable to generate a 500 server error in this case?

I see that cache items will only be evicted if write lock can be acquired. Not sure if that's a practical concern, but might the cache need to delay eviction after write lock has been released? (e.g. marking the entry as evict_requested and using that upon lock release)

If we get here, it means the dataset could not be loaded, and thus the other cache, matrix_data_cache_manager, should also not have this data cached. Maybe add a comment to that effect, unless you think it's brutally obvious. :)

Out-of-scope for this PR, but might a tombstoned dataset be removed from the matrix_data_cache_manager? Any need for maintaining its data? I assume it's worth keeping the metadata cached for tombstoned datasets.

re tombstoned datasets we want to store that information for redirects back to the dataportal in the case that someone finds the url for a deleted dataset from an external site (from a publication or other none data portal source)

re eviction with lock, the metadata cache only stores the path to the dataset so it is unlikely that it will be locked. If by some chance it is, this code path is used for every endpoint so it will basically be called immediately after. But let me know if you think this is an antipattern or has the potential to cause issues

I dontt think the call should raise issues but I can wrap it in a try except and log any errors just in case

thx for explanations, no further concerns; adding the extra try/except sounds good.

atolopko-czi · 2021-09-28T14:01:40Z

server/data_common/cache.py

+    def evict_by_key(self, cache_key: str):
+        evict = self.data.get(cache_key, None)
+        if evict:
+            self.evict_data(to_del=[(cache_key, evict)])


Just curious, why does evict_data need anything more than the cache_key?

it could probably be refactored to only take the cache_key (and use that to lookup the cache item) but currently it calls the attempt_delete function on the cache item in order to call the delte/cleanup on the datasets

atolopko-czi · 2021-09-28T14:08:08Z

server/tests/unit/common/test_api.py

+        bad_response = self.client.get(url)
+        self.assertEqual(bad_response.status_code, 404)
+        self.assertEqual(mock_expire.call_count, 1)
+        response_body_good = {


is this happy path test, below, not already being tested elsewhere?

nit: add a newline above this to separate the logical tests, or introduce a new test method for clarity

I wanted to check that the metadata api is called after expiring the cache. This code path is probably tested elsewhere but I wanted to be explicit about the expected behavior when an error causes the cache to be expired

MDunitz · 2021-09-28T18:54:47Z

Changes

modify cache code. Update dataset cache to use path to dataset as the cache key (instead of the explorer_url). Invalidate items in the metadata cache when particular errors are raised.
Update tests to use symlinks to allow for dataset cache key change

So the motivation for this change is to ensure cache provides the latest data if/when an Explorer URL-to-S3 Path mapping is no longer valid for a dataset? When is this occurring in practice?

This happens when a dataset is replaced or deleted in revision and that revision is pubished

MDunitz requested review from ebezzi and atolopko-czi September 27, 2021 22:35

atolopko-czi requested changes Sep 28, 2021

View reviewed changes

atolopko-czi approved these changes Sep 29, 2021

View reviewed changes

MDunitz added 5 commits September 30, 2021 09:44

fix: invalidate metadata cache when dataset fails to load

dca1a55

cleanup

34f300d

lint

ed9a12b

fix invalidation

aff02ed

updates for pr comments

5d8fb58

MDunitz force-pushed the dunitz/cache-invalidation branch from 70e4c55 to 5d8fb58 Compare September 30, 2021 16:46

MDunitz and others added 2 commits September 30, 2021 10:29

fix test

4ba5a25

Merge branch 'main' into dunitz/cache-invalidation

db5f22d

MDunitz merged commit db9f875 into main Sep 30, 2021

MDunitz deleted the dunitz/cache-invalidation branch September 30, 2021 17:58

czi-github-helper bot mentioned this pull request Jun 20, 2024

chore(main): release 1.0.0 #995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: invalidate metadata cache when dataset fails to load #87

fix: invalidate metadata cache when dataset fails to load #87

MDunitz commented Sep 27, 2021 •

edited

Loading

codecov bot commented Sep 27, 2021 •

edited

Loading

atolopko-czi commented Sep 28, 2021 •

edited

Loading

Changes

atolopko-czi left a comment

atolopko-czi Sep 28, 2021

atolopko-czi Sep 28, 2021

atolopko-czi Sep 28, 2021

atolopko-czi Sep 28, 2021

atolopko-czi Sep 28, 2021

MDunitz Sep 28, 2021

MDunitz Sep 28, 2021

MDunitz Sep 28, 2021

atolopko-czi Sep 29, 2021

atolopko-czi Sep 28, 2021

MDunitz Sep 30, 2021

atolopko-czi Sep 28, 2021

atolopko-czi Sep 28, 2021

MDunitz Sep 30, 2021

MDunitz commented Sep 28, 2021

Changes

fix: invalidate metadata cache when dataset fails to load #87

fix: invalidate metadata cache when dataset fails to load #87

Conversation

MDunitz commented Sep 27, 2021 • edited Loading

Reviewers

Changes

codecov bot commented Sep 27, 2021 • edited Loading

Codecov Report

atolopko-czi commented Sep 28, 2021 • edited Loading

Changes

atolopko-czi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MDunitz commented Sep 28, 2021

Changes

MDunitz commented Sep 27, 2021 •

edited

Loading

codecov bot commented Sep 27, 2021 •

edited

Loading

atolopko-czi commented Sep 28, 2021 •

edited

Loading