From d1559a6ed1988fb07a75355edf6b45511050c799 Mon Sep 17 00:00:00 2001 From: root Date: Mon, 15 Mar 2021 11:26:27 +0000 Subject: [PATCH 01/21] #209: Add client caching function - This uses an LRU caching algorithm to determine which client objects should remain cached when the cache is full --- datagateway_api/common/icat/helpers.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 73472cc9..4c53d247 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -1,5 +1,5 @@ from datetime import datetime, timedelta -from functools import wraps +from functools import lru_cache, wraps import logging import icat.client @@ -54,8 +54,8 @@ def requires_session_id(method): @wraps(method) def wrapper_requires_session(*args, **kwargs): try: - client = create_client() - client.sessionId = args[1] + client = get_cached_client(args[1]) + # Client object put into kwargs so it can be accessed by backend functions kwargs["client"] = client @@ -72,6 +72,18 @@ def wrapper_requires_session(*args, **kwargs): return wrapper_requires_session +@lru_cache(maxsize=28) +def get_cached_client(session_id): + """ + TODO - Add docstring + """ + log.debug(f"Caching, session ID: {session_id}") + client = create_client() + client.sessionId = session_id + + return client + + def create_client(): client = icat.client.Client( config.get_icat_url(), checkCert=config.get_icat_check_cert(), From a81c1455db8186a5a559f7d0b40300723b870649 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Mon, 15 Mar 2021 11:31:24 +0000 Subject: [PATCH 02/21] #209: Change ICATSessionError exception to output exception message - When running the API, it was difficult to distinguish between the AuthenticationError affected in this commit, and the one a couple of lines above it --- datagateway_api/common/icat/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 4c53d247..51d9d7f2 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -66,8 +66,8 @@ def wrapper_requires_session(*args, **kwargs): raise AuthenticationError("Forbidden") else: return method(*args, **kwargs) - except ICATSessionError: - raise AuthenticationError("Forbidden") + except ICATSessionError as e: + raise AuthenticationError(e) return wrapper_requires_session From 10fceff7fa96647a95b98497334a030fbb859ece Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Mon, 15 Mar 2021 11:50:20 +0000 Subject: [PATCH 03/21] #209: Add infomration of how credentials should be structured in request body of POST /sessions --- datagateway_api/common/backend.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/datagateway_api/common/backend.py b/datagateway_api/common/backend.py index a6ca914a..7177fc3c 100644 --- a/datagateway_api/common/backend.py +++ b/datagateway_api/common/backend.py @@ -10,7 +10,9 @@ class Backend(ABC): def login(self, credentials): """ Attempt to log a user in using the provided credentials - :param credentials: The user's credentials (including mechanism) + :param credentials: The user's credentials (including mechanism). Credentials + should take the following format in JSON: + { username: "value", password: "value", mechanism: "value"} :returns: a session ID """ pass From 3c202a87ba83958c7f04b272a9792285e482ed19 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Mon, 15 Mar 2021 11:55:43 +0000 Subject: [PATCH 04/21] #209: Make login() use client cache - Extra attention should be paid to the flushing of session ID on the client object to previous users being logged out the next time backend.login() is called --- datagateway_api/common/icat/backend.py | 10 +++++++++- datagateway_api/common/icat/helpers.py | 8 ++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/datagateway_api/common/icat/backend.py b/datagateway_api/common/icat/backend.py index 2770f190..6b436c24 100644 --- a/datagateway_api/common/icat/backend.py +++ b/datagateway_api/common/icat/backend.py @@ -9,6 +9,7 @@ create_client, create_entities, delete_entity_by_id, + get_cached_client, get_count_with_filters, get_entity_by_id, get_entity_with_filters, @@ -39,7 +40,9 @@ def __init__(self): def login(self, credentials): log.info("Logging in to get session ID") - client = create_client() + # There is no session ID required for this endpoint, a client object will be + # fetched from cache with a blank `sessionId` attribute + client = get_cached_client(None) # Syntax for Python ICAT login_details = { @@ -48,6 +51,11 @@ def login(self, credentials): } try: session_id = client.login(credentials["mechanism"], login_details) + # Flushing client's session ID so the session ID returned in this request + # won't be logged out next time `client.login()` is used in this function. + # `login()` calls `self.logout()` if `sessionId` is set + client.sessionId = None + return session_id except ICATSessionError: raise AuthenticationError("User credentials are incorrect") diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 51d9d7f2..7cc5bb1d 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -77,9 +77,13 @@ def get_cached_client(session_id): """ TODO - Add docstring """ - log.debug(f"Caching, session ID: {session_id}") client = create_client() - client.sessionId = session_id + + # `session_id` of None suggests this function is being called from an endpoint that + # doesn't use the `requires_session_id` decorator (e.g. POST /sessions) + log.info("Caching, session ID: %s", session_id) + if session_id: + client.sessionId = session_id return client From fb2009590a028f3f31deb9f4337b51775fc5b143 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Mon, 15 Mar 2021 12:37:17 +0000 Subject: [PATCH 05/21] #209: Add client cache size as a configurable option --- config.json.example | 1 + datagateway_api/common/config.py | 6 ++++++ datagateway_api/common/icat/helpers.py | 2 +- test/test_config.py | 10 ++++++++++ 4 files changed, 18 insertions(+), 1 deletion(-) diff --git a/config.json.example b/config.json.example index d44534ec..deae7156 100644 --- a/config.json.example +++ b/config.json.example @@ -1,5 +1,6 @@ { "backend": "db", + "client_cache_size": 28, "DB_URL": "mysql+pymysql://icatdbuser:icatdbuserpw@localhost:3306/icatdb", "ICAT_URL": "https://localhost:8181", "icat_check_cert": false, diff --git a/datagateway_api/common/config.py b/datagateway_api/common/config.py index 9b5090a4..d45507c7 100644 --- a/datagateway_api/common/config.py +++ b/datagateway_api/common/config.py @@ -34,6 +34,12 @@ def set_backend_type(self, backend_type): """ self.config["backend"] = backend_type + def get_client_cache_size(self): + try: + return self.config["client_cache_size"] + except KeyError: + sys.exit("Missing config value, client_cache_size") + def get_db_url(self): try: return self.config["DB_URL"] diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 7cc5bb1d..018be84e 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -72,7 +72,7 @@ def wrapper_requires_session(*args, **kwargs): return wrapper_requires_session -@lru_cache(maxsize=28) +@lru_cache(maxsize=config.get_client_cache_size()) def get_cached_client(session_id): """ TODO - Add docstring diff --git a/test/test_config.py b/test/test_config.py index 4d5da769..a3a4a910 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -30,6 +30,16 @@ def test_invalid_backend_type(self, invalid_config): invalid_config.get_backend_type() +class TestGetClientCacheSize: + def test_valid_client_cache_size(self, valid_config): + cache_size = valid_config.get_client_cache_size() + assert cache_size == 28 + + def test_invalid_client_cache_size(self, invalid_config): + with pytest.raises(SystemExit): + invalid_config.get_client_cache_size() + + class TestGetDBURL: def test_valid_db_url(self, valid_config): db_url = valid_config.get_db_url() From 74f1edd78b56b10618ec1c57f8016eb6b5073ad7 Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Mon, 29 Mar 2021 14:32:24 +0000 Subject: [PATCH 06/21] #119: Implement client object pool using LRU cache to hold 'in use' clients - LRU cache allows recently used client objects to be kept around. This means there's a 1 client object to 1 session ID ratio, more resource sensitive than a 1 client object for every request ratio - Workflow: client pool is created at startup, incoming request fetches client from LRU cache, which pulls a client from the pool. Client is kept in the cache until it becomes least recently used, at which point it's then put back into the pool - POST to /sessions uses the same workflow, passing a (pretend) session ID of None to get the same client from the cache each time - The first time a new session ID is passed in the request headers, there is no slowdown in the request like before (where a client was being created for that ID). The client is fetched from the pool - I've ran this commit against some e2e tests on the frontend and the performance from the API was good, similar to the branch which uses a single client object, passed around using kwargs. I have no concerns regarding a 'slow API' with the pool and cache - This is just a rough proof of concept, there's lots of cleaning up to do, including making the resource stats on the pool accurate and not just passing the default ones each time. One potential solution is to make something similar to the `Executor` class in the pool library. I experiemented with this (think the class I mocked up is in this commit?) but I wanted to get a basic example working before worrying about the stats (which the API doesn't make use of, but it might be useful to keep accurate stats if they ever need to be logged out. All part of the cleanup process - This doesn't use the context manager as this wouldn't allow me to implement the LRU cache in the way I have --- datagateway_api/common/icat/backend.py | 13 +++- datagateway_api/common/icat/helpers.py | 16 +++-- .../common/icat/icat_client_pool.py | 69 +++++++++++++++++++ datagateway_api/src/api_start_utils.py | 28 +++++--- .../src/resources/entities/entity_endpoint.py | 35 ++++++---- .../non_entities/sessions_endpoints.py | 15 ++-- .../table_endpoints/table_endpoints.py | 12 ++-- poetry.lock | 65 ++++++++++++----- pyproject.toml | 4 +- 9 files changed, 200 insertions(+), 57 deletions(-) create mode 100644 datagateway_api/common/icat/icat_client_pool.py diff --git a/datagateway_api/common/icat/backend.py b/datagateway_api/common/icat/backend.py index 6b436c24..374ebc94 100644 --- a/datagateway_api/common/icat/backend.py +++ b/datagateway_api/common/icat/backend.py @@ -38,11 +38,13 @@ class PythonICATBackend(Backend): def __init__(self): pass - def login(self, credentials): + def login(self, credentials, **kwargs): log.info("Logging in to get session ID") + client_pool = kwargs.get("client_pool") + # There is no session ID required for this endpoint, a client object will be # fetched from cache with a blank `sessionId` attribute - client = get_cached_client(None) + client = get_cached_client(None, client_pool) # Syntax for Python ICAT login_details = { @@ -82,7 +84,12 @@ def logout(self, session_id, **kwargs): @requires_session_id @queries_records def get_with_filters(self, session_id, entity_type, filters, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() + # TODO - Pool only needed for logging + client_pool = kwargs.get("client_pool") + log.debug(f"Pool Size: {client_pool.get_pool_size()}") + + client = kwargs.get("client") + client.sessionId = session_id return get_entity_with_filters(client, entity_type, filters) @requires_session_id diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 018be84e..7978dbbf 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -1,7 +1,8 @@ from datetime import datetime, timedelta -from functools import lru_cache, wraps +from functools import wraps import logging +from cachetools import cached import icat.client from icat.entities import getTypeMap from icat.exception import ( @@ -26,6 +27,7 @@ PythonICATLimitFilter, PythonICATWhereFilter, ) +from datagateway_api.common.icat.icat_client_pool import ExtendedLRUCache from datagateway_api.common.icat.query import ICATQuery @@ -54,8 +56,10 @@ def requires_session_id(method): @wraps(method) def wrapper_requires_session(*args, **kwargs): try: - client = get_cached_client(args[1]) + client_pool = kwargs.get("client_pool") + client = get_cached_client(args[1], client_pool) + client.sessionId = args[1] # Client object put into kwargs so it can be accessed by backend functions kwargs["client"] = client @@ -72,12 +76,14 @@ def wrapper_requires_session(*args, **kwargs): return wrapper_requires_session -@lru_cache(maxsize=config.get_client_cache_size()) -def get_cached_client(session_id): +@cached(cache=ExtendedLRUCache()) +def get_cached_client(session_id, client_pool): """ TODO - Add docstring """ - client = create_client() + + # Get a client from the pool + client, stats = client_pool._get_resource() # `session_id` of None suggests this function is being called from an endpoint that # doesn't use the `requires_session_id` decorator (e.g. POST /sessions) diff --git a/datagateway_api/common/icat/icat_client_pool.py b/datagateway_api/common/icat/icat_client_pool.py new file mode 100644 index 00000000..2ceb797c --- /dev/null +++ b/datagateway_api/common/icat/icat_client_pool.py @@ -0,0 +1,69 @@ +import logging + +from cachetools.func import _cache +from cachetools.lru import LRUCache + +from icat.client import Client +from object_pool import ObjectPool + +from datagateway_api.common.config import config + +log = logging.getLogger() + + +class ICATClient(Client): + """Wrapper class to allow an object pool of client objects to be created""" + + def __init__(self): + super().__init__(config.get_icat_url(), checkCert=config.get_icat_check_cert()) + self.autoLogout = False + + def clean_up(self): + """ + Allows object pool to cleanup the client's resources, using the existing Python + ICAT functionality + """ + super().cleanup() + + +def create_client_pool(): + return ObjectPool( + ICATClient, min_init=5, max_capacity=20, max_reusable=0, expires=0, + ) + + +class ClientPoolExecutor(ObjectPool.Executor): + """TODO""" + + def __init__(self, klass): + # klass is the instance of object pool + self.__pool = klass + self.client, self.resource_stats = None + + def get_client(self): + self.client, self.resource_stats = self.__pool._get_resource() + return self.client + + def release_client(self): + self.__pool._queue_resource(self.client, self.resource_stats) + + +def get_executor(client_pool): + return ClientPoolExecutor(client_pool) + + +class ExtendedLRUCache(LRUCache): + def __init__(self): + super().__init__(maxsize=8) + + def popitem(self): + key, client = super().popitem() + session_id, client_pool = key + log.debug(f"Item popped from LRU cache: {key}, {client}") + # Put client back into pool + # Passes in default stats for now, though these aren't used in the API + client_pool._queue_resource(client, client_pool._get_default_stats()) + + +def my_lru_cache(): + return _cache(ExtendedLRUCache()) diff --git a/datagateway_api/src/api_start_utils.py b/datagateway_api/src/api_start_utils.py index fef5f2fe..ba412ca4 100644 --- a/datagateway_api/src/api_start_utils.py +++ b/datagateway_api/src/api_start_utils.py @@ -9,6 +9,7 @@ from datagateway_api.common.backends import create_backend from datagateway_api.common.config import config +from datagateway_api.common.icat.icat_client_pool import create_client_pool from datagateway_api.src.resources.entities.entity_endpoint import ( get_count_endpoint, get_endpoint, @@ -72,27 +73,32 @@ def create_api_endpoints(flask_app, api, spec): backend = create_backend(backend_type) + if backend_type == "python_icat": + # Create client pool + # TODO - Protect against other backends + icat_client_pool = create_client_pool() + for entity_name in endpoints: get_endpoint_resource = get_endpoint( - entity_name, endpoints[entity_name], backend, + entity_name, endpoints[entity_name], backend, client_pool=icat_client_pool, ) api.add_resource(get_endpoint_resource, f"/{entity_name.lower()}") spec.path(resource=get_endpoint_resource, api=api) get_id_endpoint_resource = get_id_endpoint( - entity_name, endpoints[entity_name], backend, + entity_name, endpoints[entity_name], backend, client_pool=icat_client_pool, ) api.add_resource(get_id_endpoint_resource, f"/{entity_name.lower()}/") spec.path(resource=get_id_endpoint_resource, api=api) get_count_endpoint_resource = get_count_endpoint( - entity_name, endpoints[entity_name], backend, + entity_name, endpoints[entity_name], backend, client_pool=icat_client_pool, ) api.add_resource(get_count_endpoint_resource, f"/{entity_name.lower()}/count") spec.path(resource=get_count_endpoint_resource, api=api) get_find_one_endpoint_resource = get_find_one_endpoint( - entity_name, endpoints[entity_name], backend, + entity_name, endpoints[entity_name], backend, client_pool=icat_client_pool, ) api.add_resource( get_find_one_endpoint_resource, f"/{entity_name.lower()}/findone", @@ -100,19 +106,21 @@ def create_api_endpoints(flask_app, api, spec): spec.path(resource=get_find_one_endpoint_resource, api=api) # Session endpoint - session_endpoint_resource = session_endpoints(backend) + session_endpoint_resource = session_endpoints(backend, client_pool=icat_client_pool) api.add_resource(session_endpoint_resource, "/sessions") spec.path(resource=session_endpoint_resource, api=api) # Table specific endpoints - instrument_facility_cycle_resource = instrument_facility_cycles_endpoint(backend) + instrument_facility_cycle_resource = instrument_facility_cycles_endpoint( + backend, client_pool=icat_client_pool + ) api.add_resource( instrument_facility_cycle_resource, "/instruments//facilitycycles", ) spec.path(resource=instrument_facility_cycle_resource, api=api) count_instrument_facility_cycle_res = count_instrument_facility_cycles_endpoint( - backend, + backend, client_pool=icat_client_pool, ) api.add_resource( count_instrument_facility_cycle_res, @@ -120,7 +128,9 @@ def create_api_endpoints(flask_app, api, spec): ) spec.path(resource=count_instrument_facility_cycle_res, api=api) - instrument_investigation_resource = instrument_investigation_endpoint(backend) + instrument_investigation_resource = instrument_investigation_endpoint( + backend, client_pool=icat_client_pool, + ) api.add_resource( instrument_investigation_resource, "/instruments//facilitycycles//investigations", @@ -128,7 +138,7 @@ def create_api_endpoints(flask_app, api, spec): spec.path(resource=instrument_investigation_resource, api=api) count_instrument_investigation_resource = count_instrument_investigation_endpoint( - backend, + backend, client_pool=icat_client_pool, ) api.add_resource( count_instrument_investigation_resource, diff --git a/datagateway_api/src/resources/entities/entity_endpoint.py b/datagateway_api/src/resources/entities/entity_endpoint.py index ef26e844..2444aff6 100644 --- a/datagateway_api/src/resources/entities/entity_endpoint.py +++ b/datagateway_api/src/resources/entities/entity_endpoint.py @@ -7,7 +7,7 @@ ) -def get_endpoint(name, entity_type, backend): +def get_endpoint(name, entity_type, backend, **kwargs): """ Given an entity name generate a flask_restful Resource class. In main.py these generated classes are registered with the api e.g @@ -31,6 +31,7 @@ def get(self): get_session_id_from_auth_header(), entity_type, get_filters_from_query_string(), + **kwargs, ), 200, ) @@ -72,7 +73,10 @@ def get(self): def post(self): return ( backend.create( - get_session_id_from_auth_header(), entity_type, request.json, + get_session_id_from_auth_header(), + entity_type, + request.json, + **kwargs, ), 200, ) @@ -115,7 +119,10 @@ def post(self): def patch(self): return ( backend.update( - get_session_id_from_auth_header(), entity_type, request.json, + get_session_id_from_auth_header(), + entity_type, + request.json, + **kwargs, ), 200, ) @@ -159,7 +166,7 @@ def patch(self): return Endpoint -def get_id_endpoint(name, entity_type, backend): +def get_id_endpoint(name, entity_type, backend, **kwargs): """ Given an entity name generate a flask_restful Resource class. In main.py these generated classes are registered with the api e.g @@ -180,7 +187,7 @@ class EndpointWithID(Resource): def get(self, id_): return ( backend.get_with_id( - get_session_id_from_auth_header(), entity_type, id_, + get_session_id_from_auth_header(), entity_type, id_, **kwargs, ), 200, ) @@ -216,7 +223,9 @@ def get(self, id_): """ def delete(self, id_): - backend.delete_with_id(get_session_id_from_auth_header(), entity_type, id_) + backend.delete_with_id( + get_session_id_from_auth_header(), entity_type, id_, **kwargs, + ) return "", 204 delete.__doc__ = f""" @@ -248,8 +257,10 @@ def delete(self, id_): def patch(self, id_): session_id = get_session_id_from_auth_header() - backend.update_with_id(session_id, entity_type, id_, request.json) - return backend.get_with_id(session_id, entity_type, id_), 200 + backend.update_with_id( + session_id, entity_type, id_, request.json, **kwargs, + ) + return backend.get_with_id(session_id, entity_type, id_, **kwargs,), 200 patch.__doc__ = f""" --- @@ -293,7 +304,7 @@ def patch(self, id_): return EndpointWithID -def get_count_endpoint(name, entity_type, backend): +def get_count_endpoint(name, entity_type, backend, **kwargs): """ Given an entity name generate a flask_restful Resource class. In main.py these generated classes are registered with the api e.g @@ -313,7 +324,7 @@ def get(self): filters = get_filters_from_query_string() return ( backend.count_with_filters( - get_session_id_from_auth_header(), entity_type, filters, + get_session_id_from_auth_header(), entity_type, filters, **kwargs, ), 200, ) @@ -350,7 +361,7 @@ def get(self): return CountEndpoint -def get_find_one_endpoint(name, entity_type, backend): +def get_find_one_endpoint(name, entity_type, backend, **kwargs): """ Given an entity name generate a flask_restful Resource class. In main.py these generated classes are registered with the api e.g @@ -372,7 +383,7 @@ def get(self): filters = get_filters_from_query_string() return ( backend.get_one_with_filters( - get_session_id_from_auth_header(), entity_type, filters, + get_session_id_from_auth_header(), entity_type, filters, **kwargs, ), 200, ) diff --git a/datagateway_api/src/resources/non_entities/sessions_endpoints.py b/datagateway_api/src/resources/non_entities/sessions_endpoints.py index 7e5b2a6b..2e263908 100644 --- a/datagateway_api/src/resources/non_entities/sessions_endpoints.py +++ b/datagateway_api/src/resources/non_entities/sessions_endpoints.py @@ -10,7 +10,7 @@ log = logging.getLogger() -def session_endpoints(backend): +def session_endpoints(backend, **kwargs): """ Generate a flask_restful Resource class using the configured backend. In main.py these generated classes are registered with the api e.g. @@ -74,7 +74,7 @@ def post(self): if not ("mechanism" in request.json): request.json["mechanism"] = "simple" try: - return {"sessionID": backend.login(request.json)}, 201 + return {"sessionID": backend.login(request.json, **kwargs)}, 201 except AuthenticationError: return "Forbidden", 403 @@ -99,7 +99,7 @@ def delete(self): 404: description: Not Found - Unable to find session ID """ - backend.logout(get_session_id_from_auth_header()) + backend.logout(get_session_id_from_auth_header(), **kwargs) return "", 200 def get(self): @@ -136,7 +136,12 @@ def get(self): 403: description: Forbidden - The session ID provided is invalid """ - return backend.get_session_details(get_session_id_from_auth_header()), 200 + return ( + backend.get_session_details( + get_session_id_from_auth_header(), **kwargs + ), + 200, + ) def put(self): """ @@ -161,6 +166,6 @@ def put(self): 403: description: Forbidden - The session ID provided is invalid """ - return backend.refresh(get_session_id_from_auth_header()), 200 + return backend.refresh(get_session_id_from_auth_header(), **kwargs), 200 return Sessions diff --git a/datagateway_api/src/resources/table_endpoints/table_endpoints.py b/datagateway_api/src/resources/table_endpoints/table_endpoints.py index 3c73c406..ee5819b3 100644 --- a/datagateway_api/src/resources/table_endpoints/table_endpoints.py +++ b/datagateway_api/src/resources/table_endpoints/table_endpoints.py @@ -6,7 +6,7 @@ ) -def instrument_facility_cycles_endpoint(backend): +def instrument_facility_cycles_endpoint(backend, **kwargs): """ Generate a flask_restful Resource class using the configured backend. In main.py these generated classes are registered with the api e.g. @@ -66,6 +66,7 @@ def get(self, id_): get_session_id_from_auth_header(), id_, get_filters_from_query_string(), + **kwargs, ), 200, ) @@ -73,7 +74,7 @@ def get(self, id_): return InstrumentsFacilityCycles -def count_instrument_facility_cycles_endpoint(backend): +def count_instrument_facility_cycles_endpoint(backend, **kwargs): """ Generate a flask_restful Resource class using the configured backend. In main.py these generated classes are registered with the api e.g. @@ -126,6 +127,7 @@ def get(self, id_): get_session_id_from_auth_header(), id_, get_filters_from_query_string(), + **kwargs, ), 200, ) @@ -133,7 +135,7 @@ def get(self, id_): return InstrumentsFacilityCyclesCount -def instrument_investigation_endpoint(backend): +def instrument_investigation_endpoint(backend, **kwargs): """ Generate a flask_restful Resource class using the configured backend. In main.py these generated classes are registered with the api e.g. @@ -201,6 +203,7 @@ def get(self, instrument_id, cycle_id): instrument_id, cycle_id, get_filters_from_query_string(), + **kwargs, ), 200, ) @@ -208,7 +211,7 @@ def get(self, instrument_id, cycle_id): return InstrumentsFacilityCyclesInvestigations -def count_instrument_investigation_endpoint(backend): +def count_instrument_investigation_endpoint(backend, **kwargs): """ Generate a flask_restful Resource class using the configured backend. In main.py these generated classes are registered with the api e.g. @@ -270,6 +273,7 @@ def get(self, instrument_id, cycle_id): instrument_id, cycle_id, get_filters_from_query_string(), + **kwargs, ), 200, ) diff --git a/poetry.lock b/poetry.lock index b9d46a1d..cb01beef 100644 --- a/poetry.lock +++ b/poetry.lock @@ -87,6 +87,14 @@ typed-ast = ">=1.4.0" [package.extras] d = ["aiohttp (>=3.3.2)", "aiohttp-cors"] +[[package]] +name = "cachetools" +version = "4.2.1" +description = "Extensible memoizing collections and decorators" +category = "main" +optional = false +python-versions = "~=3.5" + [[package]] name = "certifi" version = "2020.11.8" @@ -462,17 +470,6 @@ python-versions = ">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*" pyparsing = ">=2.0.2" six = "*" -[[package]] -name = "pathlib2" -version = "2.3.5" -description = "Object-oriented filesystem paths" -category = "dev" -optional = false -python-versions = "*" - -[package.dependencies] -six = "*" - [[package]] name = "pathspec" version = "0.8.1" @@ -546,6 +543,14 @@ category = "dev" optional = false python-versions = ">=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*" +[[package]] +name = "py-object-pool" +version = "1.1" +description = "Object pool creation library" +category = "main" +optional = false +python-versions = ">=3.6" + [[package]] name = "pycodestyle" version = "2.6.0" @@ -597,7 +602,6 @@ colorama = {version = "*", markers = "sys_platform == \"win32\""} importlib-metadata = {version = ">=0.12", markers = "python_version < \"3.8\""} iniconfig = "*" packaging = "*" -pathlib2 = {version = ">=2.2.0", markers = "python_version < \"3.6\""} pluggy = ">=0.12,<1.0" py = ">=1.8.2" toml = "*" @@ -828,8 +832,8 @@ testing = ["pytest (>=3.5,!=3.7.3)", "pytest-checkdocs (>=1.2.3)", "pytest-flake [metadata] lock-version = "1.1" -python-versions = "^3.5" -content-hash = "db66f1e49ef4c5f9929c189d886b7c6b7d4306044a31e0eed7993d3ab6e9baab" +python-versions = "^3.6" +content-hash = "c5c7047d58cb4192889cd82ccad7ec95218f98af8cc1357343faeb3fbd3e188d" [metadata.files] aniso8601 = [ @@ -860,6 +864,10 @@ black = [ {file = "black-19.10b0-py36-none-any.whl", hash = "sha256:1b30e59be925fafc1ee4565e5e08abef6b03fe455102883820fe5ee2e4734e0b"}, {file = "black-19.10b0.tar.gz", hash = "sha256:c2edb73a08e9e0e6f65a0e6af18b059b8b1cdd5bef997d7a0b181df93dc81539"}, ] +cachetools = [ + {file = "cachetools-4.2.1-py3-none-any.whl", hash = "sha256:1d9d5f567be80f7c07d765e21b814326d78c61eb0c3a637dffc0e5d1796cb2e2"}, + {file = "cachetools-4.2.1.tar.gz", hash = "sha256:f469e29e7aa4cff64d8de4aad95ce76de8ea1125a16c68e0d93f65c3c3dc92e9"}, +] certifi = [ {file = "certifi-2020.11.8-py2.py3-none-any.whl", hash = "sha256:1f422849db327d534e3d0c5f02a263458c3955ec0aae4ff09b95f195c59f4edd"}, {file = "certifi-2020.11.8.tar.gz", hash = "sha256:f05def092c44fbf25834a51509ef6e631dc19765ab8a57b4e7ab85531f0a9cf4"}, @@ -1026,20 +1034,39 @@ markupsafe = [ {file = "MarkupSafe-1.1.1-cp35-cp35m-win32.whl", hash = "sha256:6dd73240d2af64df90aa7c4e7481e23825ea70af4b4922f8ede5b9e35f78a3b1"}, {file = "MarkupSafe-1.1.1-cp35-cp35m-win_amd64.whl", hash = "sha256:9add70b36c5666a2ed02b43b335fe19002ee5235efd4b8a89bfcf9005bebac0d"}, {file = "MarkupSafe-1.1.1-cp36-cp36m-macosx_10_6_intel.whl", hash = "sha256:24982cc2533820871eba85ba648cd53d8623687ff11cbb805be4ff7b4c971aff"}, + {file = "MarkupSafe-1.1.1-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:d53bc011414228441014aa71dbec320c66468c1030aae3a6e29778a3382d96e5"}, {file = "MarkupSafe-1.1.1-cp36-cp36m-manylinux1_i686.whl", hash = "sha256:00bc623926325b26bb9605ae9eae8a215691f33cae5df11ca5424f06f2d1f473"}, {file = "MarkupSafe-1.1.1-cp36-cp36m-manylinux1_x86_64.whl", hash = "sha256:717ba8fe3ae9cc0006d7c451f0bb265ee07739daf76355d06366154ee68d221e"}, + {file = "MarkupSafe-1.1.1-cp36-cp36m-manylinux2010_i686.whl", hash = "sha256:3b8a6499709d29c2e2399569d96719a1b21dcd94410a586a18526b143ec8470f"}, + {file = "MarkupSafe-1.1.1-cp36-cp36m-manylinux2010_x86_64.whl", hash = "sha256:84dee80c15f1b560d55bcfe6d47b27d070b4681c699c572af2e3c7cc90a3b8e0"}, + {file = "MarkupSafe-1.1.1-cp36-cp36m-manylinux2014_aarch64.whl", hash = "sha256:b1dba4527182c95a0db8b6060cc98ac49b9e2f5e64320e2b56e47cb2831978c7"}, {file = "MarkupSafe-1.1.1-cp36-cp36m-win32.whl", hash = "sha256:535f6fc4d397c1563d08b88e485c3496cf5784e927af890fb3c3aac7f933ec66"}, {file = "MarkupSafe-1.1.1-cp36-cp36m-win_amd64.whl", hash = "sha256:b1282f8c00509d99fef04d8ba936b156d419be841854fe901d8ae224c59f0be5"}, {file = "MarkupSafe-1.1.1-cp37-cp37m-macosx_10_6_intel.whl", hash = "sha256:8defac2f2ccd6805ebf65f5eeb132adcf2ab57aa11fdf4c0dd5169a004710e7d"}, + {file = "MarkupSafe-1.1.1-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:bf5aa3cbcfdf57fa2ee9cd1822c862ef23037f5c832ad09cfea57fa846dec193"}, {file = "MarkupSafe-1.1.1-cp37-cp37m-manylinux1_i686.whl", hash = "sha256:46c99d2de99945ec5cb54f23c8cd5689f6d7177305ebff350a58ce5f8de1669e"}, {file = "MarkupSafe-1.1.1-cp37-cp37m-manylinux1_x86_64.whl", hash = "sha256:ba59edeaa2fc6114428f1637ffff42da1e311e29382d81b339c1817d37ec93c6"}, + {file = "MarkupSafe-1.1.1-cp37-cp37m-manylinux2010_i686.whl", hash = "sha256:6fffc775d90dcc9aed1b89219549b329a9250d918fd0b8fa8d93d154918422e1"}, + {file = "MarkupSafe-1.1.1-cp37-cp37m-manylinux2010_x86_64.whl", hash = "sha256:a6a744282b7718a2a62d2ed9d993cad6f5f585605ad352c11de459f4108df0a1"}, + {file = "MarkupSafe-1.1.1-cp37-cp37m-manylinux2014_aarch64.whl", hash = "sha256:195d7d2c4fbb0ee8139a6cf67194f3973a6b3042d742ebe0a9ed36d8b6f0c07f"}, {file = "MarkupSafe-1.1.1-cp37-cp37m-win32.whl", hash = "sha256:b00c1de48212e4cc9603895652c5c410df699856a2853135b3967591e4beebc2"}, {file = "MarkupSafe-1.1.1-cp37-cp37m-win_amd64.whl", hash = "sha256:9bf40443012702a1d2070043cb6291650a0841ece432556f784f004937f0f32c"}, {file = "MarkupSafe-1.1.1-cp38-cp38-macosx_10_9_x86_64.whl", hash = "sha256:6788b695d50a51edb699cb55e35487e430fa21f1ed838122d722e0ff0ac5ba15"}, {file = "MarkupSafe-1.1.1-cp38-cp38-manylinux1_i686.whl", hash = "sha256:cdb132fc825c38e1aeec2c8aa9338310d29d337bebbd7baa06889d09a60a1fa2"}, {file = "MarkupSafe-1.1.1-cp38-cp38-manylinux1_x86_64.whl", hash = "sha256:13d3144e1e340870b25e7b10b98d779608c02016d5184cfb9927a9f10c689f42"}, + {file = "MarkupSafe-1.1.1-cp38-cp38-manylinux2010_i686.whl", hash = "sha256:acf08ac40292838b3cbbb06cfe9b2cb9ec78fce8baca31ddb87aaac2e2dc3bc2"}, + {file = "MarkupSafe-1.1.1-cp38-cp38-manylinux2010_x86_64.whl", hash = "sha256:d9be0ba6c527163cbed5e0857c451fcd092ce83947944d6c14bc95441203f032"}, + {file = "MarkupSafe-1.1.1-cp38-cp38-manylinux2014_aarch64.whl", hash = "sha256:caabedc8323f1e93231b52fc32bdcde6db817623d33e100708d9a68e1f53b26b"}, {file = "MarkupSafe-1.1.1-cp38-cp38-win32.whl", hash = "sha256:596510de112c685489095da617b5bcbbac7dd6384aeebeda4df6025d0256a81b"}, {file = "MarkupSafe-1.1.1-cp38-cp38-win_amd64.whl", hash = "sha256:e8313f01ba26fbbe36c7be1966a7b7424942f670f38e666995b88d012765b9be"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-macosx_10_9_x86_64.whl", hash = "sha256:d73a845f227b0bfe8a7455ee623525ee656a9e2e749e4742706d80a6065d5e2c"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-manylinux1_i686.whl", hash = "sha256:98bae9582248d6cf62321dcb52aaf5d9adf0bad3b40582925ef7c7f0ed85fceb"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-manylinux1_x86_64.whl", hash = "sha256:2beec1e0de6924ea551859edb9e7679da6e4870d32cb766240ce17e0a0ba2014"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-manylinux2010_i686.whl", hash = "sha256:7fed13866cf14bba33e7176717346713881f56d9d2bcebab207f7a036f41b850"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-manylinux2010_x86_64.whl", hash = "sha256:6f1e273a344928347c1290119b493a1f0303c52f5a5eae5f16d74f48c15d4a85"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-manylinux2014_aarch64.whl", hash = "sha256:feb7b34d6325451ef96bc0e36e1a6c0c1c64bc1fbec4b854f4529e51887b1621"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-win32.whl", hash = "sha256:22c178a091fc6630d0d045bdb5992d2dfe14e3259760e713c490da5323866c39"}, + {file = "MarkupSafe-1.1.1-cp39-cp39-win_amd64.whl", hash = "sha256:b7d644ddb4dbd407d31ffb699f1d140bc35478da613b441c582aeb7c43838dd8"}, {file = "MarkupSafe-1.1.1.tar.gz", hash = "sha256:29872e92839765e546828bb7754a68c418d927cd064fd4708fab9fe9c8bb116b"}, ] mccabe = [ @@ -1050,10 +1077,6 @@ packaging = [ {file = "packaging-20.4-py2.py3-none-any.whl", hash = "sha256:998416ba6962ae7fbd6596850b80e17859a5753ba17c32284f67bfff33784181"}, {file = "packaging-20.4.tar.gz", hash = "sha256:4357f74f47b9c12db93624a82154e9b120fa8293699949152b22065d556079f8"}, ] -pathlib2 = [ - {file = "pathlib2-2.3.5-py2.py3-none-any.whl", hash = "sha256:0ec8205a157c80d7acc301c0b18fbd5d44fe655968f5d947b6ecef5290fc35db"}, - {file = "pathlib2-2.3.5.tar.gz", hash = "sha256:6cd9a47b597b37cc57de1c05e56fb1a1c9cc9fab04fe78c29acd090418529868"}, -] pathspec = [ {file = "pathspec-0.8.1-py2.py3-none-any.whl", hash = "sha256:aa0cb481c4041bf52ffa7b0d8fa6cd3e88a2ca4879c533c9153882ee2556790d"}, {file = "pathspec-0.8.1.tar.gz", hash = "sha256:86379d6b86d75816baba717e64b1a3a3469deb93bb76d613c9ce79edc5cb68fd"}, @@ -1082,6 +1105,10 @@ py = [ {file = "py-1.10.0-py2.py3-none-any.whl", hash = "sha256:3b80836aa6d1feeaa108e046da6423ab8f6ceda6468545ae8d02d9d58d18818a"}, {file = "py-1.10.0.tar.gz", hash = "sha256:21b81bda15b66ef5e1a777a21c4dcd9c20ad3efd0b3f817e7a809035269e1bd3"}, ] +py-object-pool = [ + {file = "py-object-pool-1.1.tar.gz", hash = "sha256:fa3a41f363a50b8bf346880bd75f45d6a0391f24a2533a140ed531316782352c"}, + {file = "py_object_pool-1.1-py3-none-any.whl", hash = "sha256:9be717f00b861bbecc45f38108a96d7251bcaba4e02b24bbcc5115ffb9d32104"}, +] pycodestyle = [ {file = "pycodestyle-2.6.0-py2.py3-none-any.whl", hash = "sha256:2295e7b2f6b5bd100585ebcb1f616591b652db8a741695b3d8f5d28bdc934367"}, {file = "pycodestyle-2.6.0.tar.gz", hash = "sha256:c58a7d2815e0e8d7972bf1803331fb0152f867bd89adf8a01dfd55085434192e"}, @@ -1131,6 +1158,8 @@ pyyaml = [ {file = "PyYAML-5.3.1-cp37-cp37m-win_amd64.whl", hash = "sha256:73f099454b799e05e5ab51423c7bcf361c58d3206fa7b0d555426b1f4d9a3eaf"}, {file = "PyYAML-5.3.1-cp38-cp38-win32.whl", hash = "sha256:06a0d7ba600ce0b2d2fe2e78453a470b5a6e000a985dd4a4e54e436cc36b0e97"}, {file = "PyYAML-5.3.1-cp38-cp38-win_amd64.whl", hash = "sha256:95f71d2af0ff4227885f7a6605c37fd53d3a106fcab511b8860ecca9fcf400ee"}, + {file = "PyYAML-5.3.1-cp39-cp39-win32.whl", hash = "sha256:ad9c67312c84def58f3c04504727ca879cb0013b2517c85a9a253f0cb6380c0a"}, + {file = "PyYAML-5.3.1-cp39-cp39-win_amd64.whl", hash = "sha256:6034f55dab5fea9e53f436aa68fa3ace2634918e8b5994d82f3621c04ff5ed2e"}, {file = "PyYAML-5.3.1.tar.gz", hash = "sha256:b8eac752c5e14d3eca0e6dd9199cd627518cb5ec06add0de9d32baeee6fe645d"}, ] regex = [ diff --git a/pyproject.toml b/pyproject.toml index 1323ded7..ff2146ab 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,7 +9,7 @@ repository = "https://github.com/ral-facilities/datagateway-api" authors = ["Matthew Richards "] [tool.poetry.dependencies] -python = "^3.5" +python = "^3.6" Flask-RESTful = "0.3.7" SQLAlchemy = "^1.3.8" PyMySQL = "1.0.2" @@ -19,6 +19,8 @@ flask-swagger-ui = "3.25.0" PyYAML = "5.3.1" python-icat = "0.17.0" suds-community = "^0.8.4" +py-object-pool = "^1.1" +cachetools = "^4.2.1" [tool.poetry.dev-dependencies] pip-tools = "5.3.1" From 416288cb8bfbc8809207baa329d7b23d2389e3ee Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Thu, 1 Apr 2021 16:09:03 +0000 Subject: [PATCH 07/21] #209: Add comments and move code to a more logical location --- datagateway_api/common/icat/helpers.py | 17 +++++++++-- .../common/icat/icat_client_pool.py | 27 +++++------------ datagateway_api/common/icat/lru_cache.py | 29 +++++++++++++++++++ 3 files changed, 51 insertions(+), 22 deletions(-) create mode 100644 datagateway_api/common/icat/lru_cache.py diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 7978dbbf..65e18b1c 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -27,7 +27,7 @@ PythonICATLimitFilter, PythonICATWhereFilter, ) -from datagateway_api.common.icat.icat_client_pool import ExtendedLRUCache +from datagateway_api.common.icat.lru_cache import ExtendedLRUCache from datagateway_api.common.icat.query import ICATQuery @@ -79,7 +79,20 @@ def wrapper_requires_session(*args, **kwargs): @cached(cache=ExtendedLRUCache()) def get_cached_client(session_id, client_pool): """ - TODO - Add docstring + Get a client from cache using session ID as the cache parameter (client_pool will + always be given the same object, so won't impact on argument hashing) + + An available client is fetched from the object pool, given a session ID, and kept + around in this cache until it becomes 'least recently used'. At this point, the + session ID is flushed and the client is returned to the pool. More details about + client handling can be found in the README + + TODO - Add details of client handling in the README + + :param session_id: The user's session ID + :type session_id: :class:`str` + :param client_pool: Client object pool used to fetch an unused client + :type client_pool: :class:`ObjectPool` """ # Get a client from the pool diff --git a/datagateway_api/common/icat/icat_client_pool.py b/datagateway_api/common/icat/icat_client_pool.py index 2ceb797c..ff3812aa 100644 --- a/datagateway_api/common/icat/icat_client_pool.py +++ b/datagateway_api/common/icat/icat_client_pool.py @@ -1,8 +1,5 @@ import logging -from cachetools.func import _cache -from cachetools.lru import LRUCache - from icat.client import Client from object_pool import ObjectPool @@ -16,6 +13,7 @@ class ICATClient(Client): def __init__(self): super().__init__(config.get_icat_url(), checkCert=config.get_icat_check_cert()) + # When clients are cleaned up, sessions won't be logged out self.autoLogout = False def clean_up(self): @@ -27,6 +25,12 @@ def clean_up(self): def create_client_pool(): + """ + Function to create an object pool for ICAT client objects + + The ObjectPool class uses the singleton design pattern + """ + return ObjectPool( ICATClient, min_init=5, max_capacity=20, max_reusable=0, expires=0, ) @@ -50,20 +54,3 @@ def release_client(self): def get_executor(client_pool): return ClientPoolExecutor(client_pool) - - -class ExtendedLRUCache(LRUCache): - def __init__(self): - super().__init__(maxsize=8) - - def popitem(self): - key, client = super().popitem() - session_id, client_pool = key - log.debug(f"Item popped from LRU cache: {key}, {client}") - # Put client back into pool - # Passes in default stats for now, though these aren't used in the API - client_pool._queue_resource(client, client_pool._get_default_stats()) - - -def my_lru_cache(): - return _cache(ExtendedLRUCache()) diff --git a/datagateway_api/common/icat/lru_cache.py b/datagateway_api/common/icat/lru_cache.py new file mode 100644 index 00000000..23ae2790 --- /dev/null +++ b/datagateway_api/common/icat/lru_cache.py @@ -0,0 +1,29 @@ +import logging + +from cachetools.lru import LRUCache + +log = logging.getLogger() + + +class ExtendedLRUCache(LRUCache): + """ + An extension to cachetools' LRUCache class to allow client objects to be pushed back + into the pool + + This version of LRU cache was chosen instead of the builtin LRU cache as it allows + for addtional actions to be added when an item leaves the cache (controlled by + `popitem()`). Since the builtin version was just a function (using a couple of + wrapper functions), adding additional functionality wasn't possible. + """ + + def __init__(self): + super().__init__(maxsize=8) + + def popitem(self): + key, client = super().popitem() + session_id, client_pool = key + log.debug(f"Item popped from LRU cache: {key}, {client}") + # TODO - Session ID should probably get flushed here? + # Put client back into pool + # Passes in default stats for now, though these aren't used in the API + client_pool._queue_resource(client, client_pool._get_default_stats()) From e4abe888a2330ed619567880f46dac5649c15456 Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Tue, 6 Apr 2021 09:06:41 +0000 Subject: [PATCH 08/21] #209: Make client handling values configurable --- config.json.example | 4 +++- datagateway_api/common/config.py | 12 ++++++++++ .../common/icat/icat_client_pool.py | 6 ++++- datagateway_api/common/icat/lru_cache.py | 4 +++- test/test_config.py | 22 ++++++++++++++++++- 5 files changed, 44 insertions(+), 4 deletions(-) diff --git a/config.json.example b/config.json.example index deae7156..5dafee39 100644 --- a/config.json.example +++ b/config.json.example @@ -1,6 +1,8 @@ { "backend": "db", - "client_cache_size": 28, + "client_cache_size": 5, + "client_pool_init_size": 2, + "client_pool_max_capacity": 5, "DB_URL": "mysql+pymysql://icatdbuser:icatdbuserpw@localhost:3306/icatdb", "ICAT_URL": "https://localhost:8181", "icat_check_cert": false, diff --git a/datagateway_api/common/config.py b/datagateway_api/common/config.py index d45507c7..adccd267 100644 --- a/datagateway_api/common/config.py +++ b/datagateway_api/common/config.py @@ -40,6 +40,18 @@ def get_client_cache_size(self): except KeyError: sys.exit("Missing config value, client_cache_size") + def get_client_pool_init_size(self): + try: + return self.config["client_pool_init_size"] + except KeyError: + sys.exit("Missing config value, client_pool_init_size") + + def get_client_pool_max_capacity(self): + try: + return self.config["client_pool_max_capacity"] + except KeyError: + sys.exit("Missing config value, client_pool_max_capacity") + def get_db_url(self): try: return self.config["DB_URL"] diff --git a/datagateway_api/common/icat/icat_client_pool.py b/datagateway_api/common/icat/icat_client_pool.py index ff3812aa..bb684570 100644 --- a/datagateway_api/common/icat/icat_client_pool.py +++ b/datagateway_api/common/icat/icat_client_pool.py @@ -32,7 +32,11 @@ def create_client_pool(): """ return ObjectPool( - ICATClient, min_init=5, max_capacity=20, max_reusable=0, expires=0, + ICATClient, + min_init=config.get_client_pool_init_size(), + max_capacity=config.get_client_pool_max_capacity(), + max_reusable=0, + expires=0, ) diff --git a/datagateway_api/common/icat/lru_cache.py b/datagateway_api/common/icat/lru_cache.py index 23ae2790..313d9250 100644 --- a/datagateway_api/common/icat/lru_cache.py +++ b/datagateway_api/common/icat/lru_cache.py @@ -2,6 +2,8 @@ from cachetools.lru import LRUCache +from datagateway_api.common.config import config + log = logging.getLogger() @@ -17,7 +19,7 @@ class ExtendedLRUCache(LRUCache): """ def __init__(self): - super().__init__(maxsize=8) + super().__init__(maxsize=config.get_client_cache_size()) def popitem(self): key, client = super().popitem() diff --git a/test/test_config.py b/test/test_config.py index a3a4a910..78e74c95 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -33,13 +33,33 @@ def test_invalid_backend_type(self, invalid_config): class TestGetClientCacheSize: def test_valid_client_cache_size(self, valid_config): cache_size = valid_config.get_client_cache_size() - assert cache_size == 28 + assert cache_size == 5 def test_invalid_client_cache_size(self, invalid_config): with pytest.raises(SystemExit): invalid_config.get_client_cache_size() +class TestGetClientPoolInitSize: + def test_valid_client_pool_init_size(self, valid_config): + pool_init_size = valid_config.get_client_pool_init_size() + assert pool_init_size == 2 + + def test_invalid_client_cache_size(self, invalid_config): + with pytest.raises(SystemExit): + invalid_config.get_client_pool_init_size() + + +class TestGetClientPoolMaxCapacity: + def test_valid_client_pool_init_size(self, valid_config): + pool_max_capacity = valid_config.get_client_pool_max_capacity() + assert pool_max_capacity == 5 + + def test_invalid_client_cache_size(self, invalid_config): + with pytest.raises(SystemExit): + invalid_config.get_client_pool_max_capacity() + + class TestGetDBURL: def test_valid_db_url(self, valid_config): db_url = valid_config.get_db_url() From 33bdcd11f617f1d1fbe10d696bedb7ebce4773b5 Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Tue, 6 Apr 2021 09:10:15 +0000 Subject: [PATCH 09/21] #209: Change 'max capacity' to 'max size' - This change should just help make things a bit clearer due to 'init size', make the terminology more similar --- config.json.example | 2 +- datagateway_api/common/config.py | 6 +++--- datagateway_api/common/icat/icat_client_pool.py | 2 +- test/test_config.py | 8 ++++---- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/config.json.example b/config.json.example index 5dafee39..45e78348 100644 --- a/config.json.example +++ b/config.json.example @@ -2,7 +2,7 @@ "backend": "db", "client_cache_size": 5, "client_pool_init_size": 2, - "client_pool_max_capacity": 5, + "client_pool_max_size": 5, "DB_URL": "mysql+pymysql://icatdbuser:icatdbuserpw@localhost:3306/icatdb", "ICAT_URL": "https://localhost:8181", "icat_check_cert": false, diff --git a/datagateway_api/common/config.py b/datagateway_api/common/config.py index adccd267..cdf3a3b6 100644 --- a/datagateway_api/common/config.py +++ b/datagateway_api/common/config.py @@ -46,11 +46,11 @@ def get_client_pool_init_size(self): except KeyError: sys.exit("Missing config value, client_pool_init_size") - def get_client_pool_max_capacity(self): + def get_client_pool_max_size(self): try: - return self.config["client_pool_max_capacity"] + return self.config["client_pool_max_size"] except KeyError: - sys.exit("Missing config value, client_pool_max_capacity") + sys.exit("Missing config value, client_pool_max_size") def get_db_url(self): try: diff --git a/datagateway_api/common/icat/icat_client_pool.py b/datagateway_api/common/icat/icat_client_pool.py index bb684570..da30598c 100644 --- a/datagateway_api/common/icat/icat_client_pool.py +++ b/datagateway_api/common/icat/icat_client_pool.py @@ -34,7 +34,7 @@ def create_client_pool(): return ObjectPool( ICATClient, min_init=config.get_client_pool_init_size(), - max_capacity=config.get_client_pool_max_capacity(), + max_capacity=config.get_client_pool_max_size(), max_reusable=0, expires=0, ) diff --git a/test/test_config.py b/test/test_config.py index 78e74c95..619ee0ea 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -50,14 +50,14 @@ def test_invalid_client_cache_size(self, invalid_config): invalid_config.get_client_pool_init_size() -class TestGetClientPoolMaxCapacity: +class TestGetClientPoolMaxSize: def test_valid_client_pool_init_size(self, valid_config): - pool_max_capacity = valid_config.get_client_pool_max_capacity() - assert pool_max_capacity == 5 + pool_max_size = valid_config.get_client_pool_max_size() + assert pool_max_size == 5 def test_invalid_client_cache_size(self, invalid_config): with pytest.raises(SystemExit): - invalid_config.get_client_pool_max_capacity() + invalid_config.get_client_pool_max_size() class TestGetDBURL: From b0350c9afc99d7b2c73ab36d7c1ef9a7991aab89 Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Tue, 6 Apr 2021 09:12:00 +0000 Subject: [PATCH 10/21] #209: Fix API on DB backend - The client pool design wasn't working on DB backend, but now it does --- datagateway_api/common/database/backend.py | 32 +++++++++++----------- datagateway_api/src/api_start_utils.py | 4 +-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/datagateway_api/common/database/backend.py b/datagateway_api/common/database/backend.py index 927e9659..f4b8409a 100644 --- a/datagateway_api/common/database/backend.py +++ b/datagateway_api/common/database/backend.py @@ -32,7 +32,7 @@ class DatabaseBackend(Backend): Class that contains functions to access and modify data in an ICAT database directly """ - def login(self, credentials): + def login(self, credentials, **kwargs): if credentials["username"] == "user" and credentials["password"] == "password": session_id = str(uuid.uuid1()) insert_row_into_table( @@ -48,84 +48,84 @@ def login(self, credentials): raise AuthenticationError("Username and password are incorrect") @requires_session_id - def get_session_details(self, session_id): + def get_session_details(self, session_id, **kwargs): return get_row_by_id(SESSION, session_id).to_dict() @requires_session_id - def refresh(self, session_id): + def refresh(self, session_id, **kwargs): return session_id @requires_session_id @queries_records - def logout(self, session_id): + def logout(self, session_id, **kwargs): return delete_row_by_id(SESSION, session_id) @requires_session_id @queries_records - def get_with_filters(self, session_id, entity_type, filters): + def get_with_filters(self, session_id, entity_type, filters, **kwargs): table = get_entity_object_from_name(entity_type) return get_rows_by_filter(table, filters) @requires_session_id @queries_records - def create(self, session_id, entity_type, data): + def create(self, session_id, entity_type, data, **kwargs): table = get_entity_object_from_name(entity_type) return create_rows_from_json(table, data) @requires_session_id @queries_records - def update(self, session_id, entity_type, data): + def update(self, session_id, entity_type, data, **kwargs): table = get_entity_object_from_name(entity_type) return patch_entities(table, data) @requires_session_id @queries_records - def get_one_with_filters(self, session_id, entity_type, filters): + def get_one_with_filters(self, session_id, entity_type, filters, **kwargs): table = get_entity_object_from_name(entity_type) return get_first_filtered_row(table, filters) @requires_session_id @queries_records - def count_with_filters(self, session_id, entity_type, filters): + def count_with_filters(self, session_id, entity_type, filters, **kwargs): table = get_entity_object_from_name(entity_type) return get_filtered_row_count(table, filters) @requires_session_id @queries_records - def get_with_id(self, session_id, entity_type, id_): + def get_with_id(self, session_id, entity_type, id_, **kwargs): table = get_entity_object_from_name(entity_type) return get_row_by_id(table, id_).to_dict() @requires_session_id @queries_records - def delete_with_id(self, session_id, entity_type, id_): + def delete_with_id(self, session_id, entity_type, id_, **kwargs): table = get_entity_object_from_name(entity_type) return delete_row_by_id(table, id_) @requires_session_id @queries_records - def update_with_id(self, session_id, entity_type, id_, data): + def update_with_id(self, session_id, entity_type, id_, data, **kwargs): table = get_entity_object_from_name(entity_type) return update_row_from_id(table, id_, data) @requires_session_id @queries_records def get_facility_cycles_for_instrument_with_filters( - self, session_id, instrument_id, filters, + self, session_id, instrument_id, filters, **kwargs, ): return get_facility_cycles_for_instrument(instrument_id, filters) @requires_session_id @queries_records def get_facility_cycles_for_instrument_count_with_filters( - self, session_id, instrument_id, filters, + self, session_id, instrument_id, filters, **kwargs, ): return get_facility_cycles_for_instrument_count(instrument_id, filters) @requires_session_id @queries_records def get_investigations_for_instrument_facility_cycle_with_filters( - self, session_id, instrument_id, facilitycycle_id, filters, + self, session_id, instrument_id, facilitycycle_id, filters, **kwargs, ): return get_investigations_for_instrument_in_facility_cycle( instrument_id, facilitycycle_id, filters, @@ -134,7 +134,7 @@ def get_investigations_for_instrument_facility_cycle_with_filters( @requires_session_id @queries_records def get_investigation_count_instrument_facility_cycle_with_filters( - self, session_id, instrument_id, facilitycycle_id, filters, + self, session_id, instrument_id, facilitycycle_id, filters, **kwargs, ): return get_investigations_for_instrument_in_facility_cycle_count( instrument_id, facilitycycle_id, filters, diff --git a/datagateway_api/src/api_start_utils.py b/datagateway_api/src/api_start_utils.py index ba412ca4..8c14ad19 100644 --- a/datagateway_api/src/api_start_utils.py +++ b/datagateway_api/src/api_start_utils.py @@ -73,9 +73,9 @@ def create_api_endpoints(flask_app, api, spec): backend = create_backend(backend_type) + icat_client_pool = None if backend_type == "python_icat": # Create client pool - # TODO - Protect against other backends icat_client_pool = create_client_pool() for entity_name in endpoints: @@ -112,7 +112,7 @@ def create_api_endpoints(flask_app, api, spec): # Table specific endpoints instrument_facility_cycle_resource = instrument_facility_cycles_endpoint( - backend, client_pool=icat_client_pool + backend, client_pool=icat_client_pool, ) api.add_resource( instrument_facility_cycle_resource, "/instruments//facilitycycles", From e0fff60d0f7fb0bcd5b401e114dba42117161295 Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Tue, 6 Apr 2021 09:14:00 +0000 Subject: [PATCH 11/21] #209: Flush session ID before putting client back into pool --- datagateway_api/common/icat/lru_cache.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/datagateway_api/common/icat/lru_cache.py b/datagateway_api/common/icat/lru_cache.py index 313d9250..6c96709f 100644 --- a/datagateway_api/common/icat/lru_cache.py +++ b/datagateway_api/common/icat/lru_cache.py @@ -25,7 +25,10 @@ def popitem(self): key, client = super().popitem() session_id, client_pool = key log.debug(f"Item popped from LRU cache: {key}, {client}") - # TODO - Session ID should probably get flushed here? - # Put client back into pool - # Passes in default stats for now, though these aren't used in the API + + # Flushing session ID so next time the client object is used, there's no issues + client.sessionId = None + + # Put client back into pool - resource stats aren't used in the API, so defaults + # are passed in client_pool._queue_resource(client, client_pool._get_default_stats()) From 400b6b5070ae361f94fe8b5d3a1ebabfa66f3a4f Mon Sep 17 00:00:00 2001 From: "matthew.richards@stfc.ac.uk" Date: Tue, 6 Apr 2021 09:19:22 +0000 Subject: [PATCH 12/21] #209: Add documentation for client handling --- README.md | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 2fc4f4a8..0d476fd2 100644 --- a/README.md +++ b/README.md @@ -650,11 +650,74 @@ the API supporting multiple authentication mechanisms. Meta attributes such as ` are dealt by Python ICAT, rather than the API. +### Client Handling +Python ICAT uses +[client objects](https://python-icat.readthedocs.io/en/stable/client.html) to +authenticate users and provide interaction to ICAT (e.g. querying icatdb). A client +object has a high creation cost (often taking several seconds), so it's unsuitable to +create a new client object at the start of each request. In a similar vein, it would +also be unsuitable to use a single client object for the entire API due to collisions +between different users. + +Client objects are handled using an +[LRU cache](https://docs.python.org/3/library/functools.html#functools.lru_cache), +fetching clients from an [object pool](https://object-pool.readthedocs.io/en/latest/) +when a new client is requested for the cache. + +#### Caching +The cache is extended from Cachetools' implementation (although the documentation for +the builtin LRU cache is more detailed, hence that's linked above) to allow for a client +object to be placed back into the object pool once it becomes 'least recently used' and +therefore is removed from the cache (in place of another item). Each cache item is +differentiated by the arguments of the function it's applied to which in this case is +the session ID. The client pool object is also passed into the function, but this is a +singleton object (mandated by the library it's implemented from) so this won't change +throughout the lifetime of the API. + +#### Pooling +The object pool has an initial pool size that will be created at startup, and a maximum +size that the pool can grow to if needed, where both values are configurable. The +clients within the pool do not expire and have unlimited reuses, so clients created at +startup can be used for the lifespan of the API. Python ICAT's `Client` class is +extended (to `ICATClient`) to rename `cleanup()` to a function name that the object pool +will recognise to clean up resources and will disable the auto logout feature to prevent +sessions from being logged out when the client is reused. + +#### Attributes of the Design +Combining caching and pooling into one design gives the following high-level results. +There is a 1 client to 1 session ID ratio, which will prevent collision between users +and doesn't require an excessive amount of resources (such as a 1 client to 1 request +ratio would). Since the object pool is created at startup, this design can cause the API +to be slow to start as the pool of object needs to be created. A rough guide would be to +multiply the configured initial pool size by around 5 or 6 seconds to get a time +estimate for pool creation. + +#### Configuring Client Handling +When configuring the cache size and the client pool, the following should be considered. +The pool's max size should be configured to the maximum number of concurrent users +expected for the API. The cache size must not exceed the pool's maximum size. If +this does happen, the cache could attempt to acquire a client from an empty pool that +cannot grow, causing the request to never respond because the API will wait +indefinitely. The pool's initial size should be configured to strike a balance of +reasonable startup time and not slowing down requests when the pool grows beyond its +initial size. NOTE: when the pool exceeds the initial size and a client is requested by +the cache, a client is created on the fly, so that request (and any others sent before +the client is created and in the cache) WILL be slow. For development, the following +settings (as also set in the example config) would allow for an acceptable startup time +but allow for multiple session IDs to be used if required. + +```json +"client_cache_size": 5, +"client_pool_init_size": 2, +"client_pool_max_size": 5, +``` + + ### ICATQuery The ICATQuery classed is in `datagateway_api.common.icat.query`. This class stores a query created with Python ICAT -[documentation for the query](https://python-icat.readthedocs.io/en/stable/query.html). -The `execute_query()` function executes the query and returns either results in either a +([documentation](https://python-icat.readthedocs.io/en/stable/query.html)). The +`execute_query()` function executes the query and returns either results in either a JSON format, or a list of [Python ICAT entity's](https://python-icat.readthedocs.io/en/stable/entity.html) (this is defined using the `return_json_formattable` flag). Other functions within that class From 7a885d20b84846361f9f3f9493013684aba65d51 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 10:01:32 +0000 Subject: [PATCH 13/21] #209: Fix linting issues --- datagateway_api/common/icat/backend.py | 4 ---- datagateway_api/common/icat/lru_cache.py | 2 +- datagateway_api/src/resources/entities/entity_endpoint.py | 2 +- .../src/resources/non_entities/sessions_endpoints.py | 2 +- 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/datagateway_api/common/icat/backend.py b/datagateway_api/common/icat/backend.py index 374ebc94..81e2c26c 100644 --- a/datagateway_api/common/icat/backend.py +++ b/datagateway_api/common/icat/backend.py @@ -84,10 +84,6 @@ def logout(self, session_id, **kwargs): @requires_session_id @queries_records def get_with_filters(self, session_id, entity_type, filters, **kwargs): - # TODO - Pool only needed for logging - client_pool = kwargs.get("client_pool") - log.debug(f"Pool Size: {client_pool.get_pool_size()}") - client = kwargs.get("client") client.sessionId = session_id return get_entity_with_filters(client, entity_type, filters) diff --git a/datagateway_api/common/icat/lru_cache.py b/datagateway_api/common/icat/lru_cache.py index 6c96709f..7d3cedfe 100644 --- a/datagateway_api/common/icat/lru_cache.py +++ b/datagateway_api/common/icat/lru_cache.py @@ -24,7 +24,7 @@ def __init__(self): def popitem(self): key, client = super().popitem() session_id, client_pool = key - log.debug(f"Item popped from LRU cache: {key}, {client}") + log.debug("Client popped from LRU cache with session: %s", session_id) # Flushing session ID so next time the client object is used, there's no issues client.sessionId = None diff --git a/datagateway_api/src/resources/entities/entity_endpoint.py b/datagateway_api/src/resources/entities/entity_endpoint.py index 2444aff6..5a278e4b 100644 --- a/datagateway_api/src/resources/entities/entity_endpoint.py +++ b/datagateway_api/src/resources/entities/entity_endpoint.py @@ -260,7 +260,7 @@ def patch(self, id_): backend.update_with_id( session_id, entity_type, id_, request.json, **kwargs, ) - return backend.get_with_id(session_id, entity_type, id_, **kwargs,), 200 + return backend.get_with_id(session_id, entity_type, id_, **kwargs), 200 patch.__doc__ = f""" --- diff --git a/datagateway_api/src/resources/non_entities/sessions_endpoints.py b/datagateway_api/src/resources/non_entities/sessions_endpoints.py index 2e263908..651f5e5c 100644 --- a/datagateway_api/src/resources/non_entities/sessions_endpoints.py +++ b/datagateway_api/src/resources/non_entities/sessions_endpoints.py @@ -138,7 +138,7 @@ def get(self): """ return ( backend.get_session_details( - get_session_id_from_auth_header(), **kwargs + get_session_id_from_auth_header(), **kwargs, ), 200, ) From a9600e945a8764c9a27fb7ff68e2aa20fdd52e01 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 10:34:33 +0000 Subject: [PATCH 14/21] #209: Remove client defensiveness - This is no longer needed due to the new solution for client handling --- datagateway_api/common/icat/backend.py | 51 ++++++++++---------------- 1 file changed, 19 insertions(+), 32 deletions(-) diff --git a/datagateway_api/common/icat/backend.py b/datagateway_api/common/icat/backend.py index 81e2c26c..83461672 100644 --- a/datagateway_api/common/icat/backend.py +++ b/datagateway_api/common/icat/backend.py @@ -6,7 +6,6 @@ from datagateway_api.common.exceptions import AuthenticationError from datagateway_api.common.helpers import queries_records from datagateway_api.common.icat.helpers import ( - create_client, create_entities, delete_entity_by_id, get_cached_client, @@ -65,95 +64,84 @@ def login(self, credentials, **kwargs): @requires_session_id def get_session_details(self, session_id, **kwargs): log.info("Getting session details for session: %s", session_id) - client = kwargs["client"] if kwargs["client"] else create_client() - return get_session_details_helper(client) + return get_session_details_helper(kwargs.get("client")) @requires_session_id def refresh(self, session_id, **kwargs): log.info("Refreshing session: %s", session_id) - client = kwargs["client"] if kwargs["client"] else create_client() - return refresh_client_session(client) + return refresh_client_session(kwargs.get("client")) @requires_session_id @queries_records def logout(self, session_id, **kwargs): log.info("Logging out of the Python ICAT client") - client = kwargs["client"] if kwargs["client"] else create_client() - return logout_icat_client(client) + return logout_icat_client(kwargs.get("client")) @requires_session_id @queries_records def get_with_filters(self, session_id, entity_type, filters, **kwargs): - client = kwargs.get("client") - client.sessionId = session_id - return get_entity_with_filters(client, entity_type, filters) + return get_entity_with_filters(kwargs.get("client"), entity_type, filters) @requires_session_id @queries_records def create(self, session_id, entity_type, data, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return create_entities(client, entity_type, data) + return create_entities(kwargs.get("client"), entity_type, data) @requires_session_id @queries_records def update(self, session_id, entity_type, data, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return update_entities(client, entity_type, data) + return update_entities(kwargs.get("client"), entity_type, data) @requires_session_id @queries_records def get_one_with_filters(self, session_id, entity_type, filters, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return get_first_result_with_filters(client, entity_type, filters) + return get_first_result_with_filters(kwargs.get("client"), entity_type, filters) @requires_session_id @queries_records def count_with_filters(self, session_id, entity_type, filters, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return get_count_with_filters(client, entity_type, filters) + return get_count_with_filters(kwargs.get("client"), entity_type, filters) @requires_session_id @queries_records def get_with_id(self, session_id, entity_type, id_, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return get_entity_by_id(client, entity_type, id_, True) + return get_entity_by_id(kwargs.get("client"), entity_type, id_, True) @requires_session_id @queries_records def delete_with_id(self, session_id, entity_type, id_, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return delete_entity_by_id(client, entity_type, id_) + return delete_entity_by_id(kwargs.get("client"), entity_type, id_) @requires_session_id @queries_records def update_with_id(self, session_id, entity_type, id_, data, **kwargs): - client = kwargs["client"] if kwargs["client"] else create_client() - return update_entity_by_id(client, entity_type, id_, data) + return update_entity_by_id(kwargs.get("client"), entity_type, id_, data) @requires_session_id @queries_records def get_facility_cycles_for_instrument_with_filters( self, session_id, instrument_id, filters, **kwargs, ): - client = kwargs["client"] if kwargs["client"] else create_client() - return get_facility_cycles_for_instrument(client, instrument_id, filters) + return get_facility_cycles_for_instrument( + kwargs.get("client"), instrument_id, filters, + ) @requires_session_id @queries_records def get_facility_cycles_for_instrument_count_with_filters( self, session_id, instrument_id, filters, **kwargs, ): - client = kwargs["client"] if kwargs["client"] else create_client() - return get_facility_cycles_for_instrument_count(client, instrument_id, filters) + return get_facility_cycles_for_instrument_count( + kwargs.get("client"), instrument_id, filters, + ) @requires_session_id @queries_records def get_investigations_for_instrument_facility_cycle_with_filters( self, session_id, instrument_id, facilitycycle_id, filters, **kwargs, ): - client = kwargs["client"] if kwargs["client"] else create_client() return get_investigations_for_instrument_in_facility_cycle( - client, instrument_id, facilitycycle_id, filters, + kwargs.get("client"), instrument_id, facilitycycle_id, filters, ) @requires_session_id @@ -161,7 +149,6 @@ def get_investigations_for_instrument_facility_cycle_with_filters( def get_investigation_count_instrument_facility_cycle_with_filters( self, session_id, instrument_id, facilitycycle_id, filters, **kwargs, ): - client = kwargs["client"] if kwargs["client"] else create_client() return get_investigations_for_instrument_in_facility_cycle_count( - client, instrument_id, facilitycycle_id, filters, + kwargs.get("client"), instrument_id, facilitycycle_id, filters, ) From 1a1064cd361c4bfb8c25e355f6cfe4c119967166 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 10:37:19 +0000 Subject: [PATCH 15/21] #209: Remove executor - This was never used and won't be needed for the solution to client handling --- .../common/icat/icat_client_pool.py | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/datagateway_api/common/icat/icat_client_pool.py b/datagateway_api/common/icat/icat_client_pool.py index da30598c..dad667e2 100644 --- a/datagateway_api/common/icat/icat_client_pool.py +++ b/datagateway_api/common/icat/icat_client_pool.py @@ -38,23 +38,3 @@ def create_client_pool(): max_reusable=0, expires=0, ) - - -class ClientPoolExecutor(ObjectPool.Executor): - """TODO""" - - def __init__(self, klass): - # klass is the instance of object pool - self.__pool = klass - self.client, self.resource_stats = None - - def get_client(self): - self.client, self.resource_stats = self.__pool._get_resource() - return self.client - - def release_client(self): - self.__pool._queue_resource(self.client, self.resource_stats) - - -def get_executor(client_pool): - return ClientPoolExecutor(client_pool) From 6acfd482c208c15ac8de94c08c9c327a4606bc93 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 11:18:28 +0000 Subject: [PATCH 16/21] #209: Remove create client function - Client creation is handled by `ICATClient` --- datagateway_api/common/icat/helpers.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 65e18b1c..1a3829bf 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -3,7 +3,6 @@ import logging from cachetools import cached -import icat.client from icat.entities import getTypeMap from icat.exception import ( ICATInternalError, @@ -14,7 +13,6 @@ ICATValidationError, ) -from datagateway_api.common.config import config from datagateway_api.common.date_handler import DateHandler from datagateway_api.common.exceptions import ( AuthenticationError, @@ -107,13 +105,6 @@ def get_cached_client(session_id, client_pool): return client -def create_client(): - client = icat.client.Client( - config.get_icat_url(), checkCert=config.get_icat_check_cert(), - ) - return client - - def get_session_details_helper(client): """ Retrieve details regarding the current session within `client` From cad5677a650b2bc6d282c9ff25cf54b510963f72 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 11:51:48 +0000 Subject: [PATCH 17/21] #209: Add tests for custom LRU cache --- test/icat/test_lru_cache.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 test/icat/test_lru_cache.py diff --git a/test/icat/test_lru_cache.py b/test/icat/test_lru_cache.py new file mode 100644 index 00000000..f413e7e1 --- /dev/null +++ b/test/icat/test_lru_cache.py @@ -0,0 +1,26 @@ +from unittest.mock import MagicMock + +from cachetools import cached + +from datagateway_api.common.config import config +from datagateway_api.common.icat.lru_cache import ExtendedLRUCache + + +class TestLRUCache: + def test_valid_cache_creation(self): + test_cache = ExtendedLRUCache() + assert test_cache.maxsize == config.get_client_cache_size() + + def test_valid_popitem(self, icat_client): + test_cache = ExtendedLRUCache() + + test_cache.popitem = MagicMock(side_effect=test_cache.popitem) + + @cached(cache=test_cache) + def get_cached_client(cache_number): + return icat_client + + for cache_number in range(config.get_client_cache_size() + 1): + get_cached_client(cache_number) + + assert test_cache.popitem.called From bdc72fbae49ef9f2622c0433444de54c8099367a Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 12:10:23 +0000 Subject: [PATCH 18/21] #209: Increase test coverage for extended cache --- test/icat/test_lru_cache.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/icat/test_lru_cache.py b/test/icat/test_lru_cache.py index f413e7e1..5b3b5822 100644 --- a/test/icat/test_lru_cache.py +++ b/test/icat/test_lru_cache.py @@ -3,6 +3,7 @@ from cachetools import cached from datagateway_api.common.config import config +from datagateway_api.common.icat.icat_client_pool import create_client_pool from datagateway_api.common.icat.lru_cache import ExtendedLRUCache @@ -13,14 +14,15 @@ def test_valid_cache_creation(self): def test_valid_popitem(self, icat_client): test_cache = ExtendedLRUCache() + test_pool = create_client_pool() test_cache.popitem = MagicMock(side_effect=test_cache.popitem) @cached(cache=test_cache) - def get_cached_client(cache_number): + def get_cached_client(cache_number, client_pool): return icat_client for cache_number in range(config.get_client_cache_size() + 1): - get_cached_client(cache_number) + get_cached_client(cache_number, test_pool) assert test_cache.popitem.called From c6e34e8e20dc0f007fbd5db52d6a1ba1b559881a Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 12:32:20 +0000 Subject: [PATCH 19/21] #209: Fix LRU cache test --- test/icat/test_lru_cache.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/icat/test_lru_cache.py b/test/icat/test_lru_cache.py index 5b3b5822..b400317e 100644 --- a/test/icat/test_lru_cache.py +++ b/test/icat/test_lru_cache.py @@ -1,6 +1,7 @@ from unittest.mock import MagicMock from cachetools import cached +from icat.client import Client from datagateway_api.common.config import config from datagateway_api.common.icat.icat_client_pool import create_client_pool @@ -12,15 +13,18 @@ def test_valid_cache_creation(self): test_cache = ExtendedLRUCache() assert test_cache.maxsize == config.get_client_cache_size() - def test_valid_popitem(self, icat_client): + def test_valid_popitem(self): test_cache = ExtendedLRUCache() test_pool = create_client_pool() + test_client = Client( + config.get_icat_url(), checkCert=config.get_icat_check_cert(), + ) test_cache.popitem = MagicMock(side_effect=test_cache.popitem) @cached(cache=test_cache) def get_cached_client(cache_number, client_pool): - return icat_client + return test_client for cache_number in range(config.get_client_cache_size() + 1): get_cached_client(cache_number, test_pool) From 4ed8061affba0cb1727bc2cbbd32ecb850d5a587 Mon Sep 17 00:00:00 2001 From: Matthew Richards Date: Tue, 6 Apr 2021 12:57:52 +0000 Subject: [PATCH 20/21] #209: Remove irrelevant TODO --- datagateway_api/common/icat/helpers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/datagateway_api/common/icat/helpers.py b/datagateway_api/common/icat/helpers.py index 1a3829bf..6be71ca3 100644 --- a/datagateway_api/common/icat/helpers.py +++ b/datagateway_api/common/icat/helpers.py @@ -85,8 +85,6 @@ def get_cached_client(session_id, client_pool): session ID is flushed and the client is returned to the pool. More details about client handling can be found in the README - TODO - Add details of client handling in the README - :param session_id: The user's session ID :type session_id: :class:`str` :param client_pool: Client object pool used to fetch an unused client From 85e52f05eeb439d39ddbd75c41418f5ef8c4bac4 Mon Sep 17 00:00:00 2001 From: Matthew Richards <32678030+MRichards99@users.noreply.github.com> Date: Tue, 6 Apr 2021 17:06:15 +0100 Subject: [PATCH 21/21] Add heading to contents --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 1db9407a..c80f7626 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,7 @@ sqlalchemy to communicate directly with ICAT's database. - [Database Backend](#database-backend) - [Mapped Classes](#mapped-classes) - [Python ICAT Backend](#python-icat-backend) + - [Client Handling](#client-handling) - [ICATQuery](#icatquery) - [Generating the OpenAPI Specification](#generating-the-openapi-specification) - [Utilities](#utilities)