OnroerendErfgoed / skosprovider_atramhasis

A skosprovider that can talk to an Atramhasis instance
http://skosprovider-atramhasis.readthedocs.org
MIT License
2 stars 0 forks source link

Add cache to Atramhasisprovider methods. #76

Closed Wim-De-Clercq closed 4 years ago

Wim-De-Clercq commented 4 years ago

Enkele opmerkingen:

De cache config is niet per instantie, maar het lijkt alsof dat zo is.

provider1 = AtramhasisProvider(..., cache_config={'backend': 'a'})
provider2 = AtramhasisProvider(..., cache_config={'backend': 'b'})

resultaat is dat beide providers backend a zullen hebben. Dit is in lijn met oeauth en andere, maar het ziet er wel vreemd uit.


Verschillende provider objecten met dezelfde url en conceptscheme delen hun cache.

provider1 = AtramhasisProvider(same)
provider2 = AtramhasisProvider(same)
# provider 1 and 2 will use the same cache.

Dit kan voor en nadelen hebben.


Staat los van deze PR, maar AtramhasisProvider.init doet geen super meer. Ik weet niet of dit intended is?

koenedaele commented 4 years ago

Staat los van deze PR, maar AtramhasisProvider.init doet geen super meer. Ik weet niet of dit intended is?

Klopt. Dat komt door #68 Anders moet je bij de init van deze class al meteen een CS gaan laden en als die call mislukt kan je het object niet instantiëren en zit je vast. Ben zelf niet geheel gelukkig met de werkwijze, maar op dit moment was het de beste oplossing. Opties zouden kunnen zijn om CS niet meer verplicht te maken in de constructor en een soort lazy load placeholder te gebruiker. Maar dat is denk ik voor een ander moment.

koenedaele commented 4 years ago

De cache config is niet per instantie, maar het lijkt alsof dat zo is.

provider1 = AtramhasisProvider(..., cache_config={'backend': 'a'})
provider2 = AtramhasisProvider(..., cache_config={'backend': 'b'})

resultaat is dat beide providers backend a zullen hebben. Dit is in lijn met oeauth en andere, maar het ziet er wel vreemd uit.

Verschillende provider objecten met dezelfde url en conceptscheme delen hun cache.

provider1 = AtramhasisProvider(same)
provider2 = AtramhasisProvider(same)
# provider 1 and 2 will use the same cache.

Dit kan voor en nadelen hebben.

Als je twee providers aanmaakt met een verschillende id (in de metadata), maar dezelfde config (base_url en scheme_id), dan heb je eigenlijk twee providers geconfigureerd die exact dezelfde calls gaan doen. Ik kan geen zinnige reden bedenken waarom je dat wil doen en het zal mogelijk ook vreemde resultaten geven bij bv. registry.get_by_uri (want twee providers kunnen die URI vinden). Daar zijn op zich geen validaties op die dat tegenhouden omdat het redelijk ver gezocht is dat je dat zou beginnen doen.

Het andere geval, waarbij je twee AtramhasisProviders hebt die naar 2 verschillende backends wijzen, kan nuttiger zijn. Dan heb je bv. onze eigen thesaurus die je héél lang wil cachend (1 maand) en een andere Atramhasis instantie die je maar 1 dag wil cachen. In crabpy hangen de caching regions vast aan de instantie waaraan ze gekoppeld zijn (https://github.com/OnroerendErfgoed/crabpy/blob/master/crabpy/gateway/crab.py#L59). Dus, daar kan je voor elke gateway een aparte memory cache hebben (maar, er is maar 1 crab service, dus eigenlijk doet je dat ook niet). En als die allemaal naar dezelfde redis cache wijzen maakt het ook niet zo veel uit. In de praktijk ken ik momenteel ook maar 1 Atramhasis instantie en dat is de onze, dus het is allemaal een beetje een theoretische discussie. Wat mij betreft is het ok op deze manier.

Wim-De-Clercq commented 4 years ago

I wonder if it's better to move the 3 caching related functions - _atramhasis_key_generator, _dont_cache_false, _cache_on_arguments - to another file...