From 022376a27d54777f186a964f843127a7c14f830f Mon Sep 17 00:00:00 2001 From: Nikhar Saxena Date: Sun, 8 May 2022 23:30:28 -0700 Subject: [PATCH 1/4] chore(tiger): Raise a different timeout exception from tiger clusters To differentiate between the cache timeout errors happening between the production clusters and tiger clusters, raise a different exception in each case. --- snuba/state/cache/abstract.py | 4 ++++ snuba/state/cache/redis/backend.py | 13 ++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/snuba/state/cache/abstract.py b/snuba/state/cache/abstract.py index eea947b1c8..59b88c373f 100644 --- a/snuba/state/cache/abstract.py +++ b/snuba/state/cache/abstract.py @@ -15,6 +15,10 @@ class ExecutionTimeoutError(ExecutionError): pass +class TigerExecutionTimeoutError(ExecutionError): + pass + + class Cache(Generic[TValue], ABC): @abstractmethod def get(self, key: str) -> Optional[TValue]: diff --git a/snuba/state/cache/redis/backend.py b/snuba/state/cache/redis/backend.py index 5c5e328bbd..1c2a891370 100644 --- a/snuba/state/cache/redis/backend.py +++ b/snuba/state/cache/redis/backend.py @@ -13,6 +13,7 @@ Cache, ExecutionError, ExecutionTimeoutError, + TigerExecutionTimeoutError, TValue, ) from snuba.utils.codecs import ExceptionAwareCodec @@ -39,6 +40,7 @@ def __init__( self.__prefix = prefix self.__codec = codec self.__executor = executor + self.__is_tiger_cache = True if "tiger" in self.__prefix else False # TODO: This should probably be lazily instantiated, rather than # automatically happening at startup. @@ -257,9 +259,14 @@ def build_notify_queue_key(task_ident: str) -> str: # If the effective timeout was the remaining task timeout, # this means that the client responsible for generating the # cache value didn't do so before it promised to. - raise ExecutionTimeoutError( - "result not available before execution deadline" - ) + if self.__is_tiger_cache: + raise TigerExecutionTimeoutError( + "result not available before execution deadline" + ) + else: + raise ExecutionTimeoutError( + "result not available before execution deadline" + ) else: # If the effective timeout was the timeout provided to this # method, that means that our timeout was stricter From ce7ece4a0efd7b8a36beff23f1c921672f260b62 Mon Sep 17 00:00:00 2001 From: Nikhar Saxena Date: Mon, 9 May 2022 11:56:06 -0700 Subject: [PATCH 2/4] Move exception type to instantiation --- snuba/state/cache/redis/backend.py | 24 +++++++----------------- snuba/web/db_query.py | 18 ++++++++++++++++-- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/snuba/state/cache/redis/backend.py b/snuba/state/cache/redis/backend.py index 1c2a891370..009eea616e 100644 --- a/snuba/state/cache/redis/backend.py +++ b/snuba/state/cache/redis/backend.py @@ -2,20 +2,14 @@ import logging import uuid from concurrent.futures import ThreadPoolExecutor -from typing import Callable, Optional +from typing import Callable, Optional, Type from pkg_resources import resource_string from redis.exceptions import ResponseError from snuba.redis import RedisClientType from snuba.state import get_config -from snuba.state.cache.abstract import ( - Cache, - ExecutionError, - ExecutionTimeoutError, - TigerExecutionTimeoutError, - TValue, -) +from snuba.state.cache.abstract import Cache, ExecutionError, TValue from snuba.utils.codecs import ExceptionAwareCodec from snuba.utils.metrics.timer import Timer from snuba.utils.serializable_exception import SerializableException @@ -35,12 +29,13 @@ def __init__( prefix: str, codec: ExceptionAwareCodec[bytes, TValue], executor: ThreadPoolExecutor, + timeout_exception: Type[Exception], ) -> None: self.__client = client self.__prefix = prefix self.__codec = codec self.__executor = executor - self.__is_tiger_cache = True if "tiger" in self.__prefix else False + self.__timeout_exception = timeout_exception # TODO: This should probably be lazily instantiated, rather than # automatically happening at startup. @@ -259,14 +254,9 @@ def build_notify_queue_key(task_ident: str) -> str: # If the effective timeout was the remaining task timeout, # this means that the client responsible for generating the # cache value didn't do so before it promised to. - if self.__is_tiger_cache: - raise TigerExecutionTimeoutError( - "result not available before execution deadline" - ) - else: - raise ExecutionTimeoutError( - "result not available before execution deadline" - ) + raise self.__timeout_exception( + "result not available before execution deadline" + ) else: # If the effective timeout was the timeout provided to this # method, that means that our timeout was stricter diff --git a/snuba/web/db_query.py b/snuba/web/db_query.py index 231156e46f..1a79ac53cf 100644 --- a/snuba/web/db_query.py +++ b/snuba/web/db_query.py @@ -33,7 +33,11 @@ from snuba.reader import Reader, Result from snuba.redis import redis_client from snuba.request.request_settings import RequestSettings -from snuba.state.cache.abstract import Cache, ExecutionTimeoutError +from snuba.state.cache.abstract import ( + Cache, + ExecutionTimeoutError, + TigerExecutionTimeoutError, +) from snuba.state.cache.redis.backend import RESULT_VALUE, RESULT_WAIT, RedisCache from snuba.state.rate_limit import ( GLOBAL_RATE_LIMIT_NAME, @@ -79,7 +83,11 @@ def encode_exception(self, value: SerializableException) -> bytes: # reader when running a query. cache_partitions: MutableMapping[str, Cache[Result]] = { DEFAULT_CACHE_PARTITION_ID: RedisCache( - redis_client, "snuba-query-cache:", ResultCacheCodec(), ThreadPoolExecutor() + redis_client, + "snuba-query-cache:", + ResultCacheCodec(), + ThreadPoolExecutor(), + ExecutionTimeoutError, ) } # This lock prevents us from initializing the cache twice. The cache is initialized @@ -362,11 +370,17 @@ def _get_cache_partition(reader: Reader) -> Cache[Result]: # during the first query. So, for the vast majority of queries, the overhead # of acquiring the lock is not needed. if partition_id not in cache_partitions: + exception = ( + TigerExecutionTimeoutError + if "tiger" in partition_id + else ExecutionTimeoutError + ) cache_partitions[partition_id] = RedisCache( redis_client, f"snuba-query-cache:{partition_id}:", ResultCacheCodec(), ThreadPoolExecutor(), + exception, ) return cache_partitions[ From d9a0ea15e43c5b575aa9ccff28898d4ceb93face Mon Sep 17 00:00:00 2001 From: Nikhar Saxena Date: Mon, 9 May 2022 14:52:16 -0700 Subject: [PATCH 3/4] Make the exception optional --- snuba/state/cache/redis/backend.py | 9 +++++++-- snuba/web/db_query.py | 6 ++++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/snuba/state/cache/redis/backend.py b/snuba/state/cache/redis/backend.py index 009eea616e..8bf5ade746 100644 --- a/snuba/state/cache/redis/backend.py +++ b/snuba/state/cache/redis/backend.py @@ -9,7 +9,12 @@ from redis.exceptions import ResponseError from snuba.redis import RedisClientType from snuba.state import get_config -from snuba.state.cache.abstract import Cache, ExecutionError, TValue +from snuba.state.cache.abstract import ( + Cache, + ExecutionError, + ExecutionTimeoutError, + TValue, +) from snuba.utils.codecs import ExceptionAwareCodec from snuba.utils.metrics.timer import Timer from snuba.utils.serializable_exception import SerializableException @@ -29,7 +34,7 @@ def __init__( prefix: str, codec: ExceptionAwareCodec[bytes, TValue], executor: ThreadPoolExecutor, - timeout_exception: Type[Exception], + timeout_exception: Optional[Type[Exception]] = ExecutionTimeoutError, ) -> None: self.__client = client self.__prefix = prefix diff --git a/snuba/web/db_query.py b/snuba/web/db_query.py index 1a79ac53cf..c7b1da830f 100644 --- a/snuba/web/db_query.py +++ b/snuba/web/db_query.py @@ -87,7 +87,6 @@ def encode_exception(self, value: SerializableException) -> bytes: "snuba-query-cache:", ResultCacheCodec(), ThreadPoolExecutor(), - ExecutionTimeoutError, ) } # This lock prevents us from initializing the cache twice. The cache is initialized @@ -601,7 +600,10 @@ def raw_query( errors.ErrorCodes.NETWORK_ERROR, ): sentry_sdk.set_tag("timeout", "network") - elif isinstance(cause, (TimeoutError, ExecutionTimeoutError)): + elif isinstance( + cause, + (TimeoutError, ExecutionTimeoutError, TigerExecutionTimeoutError), + ): if scope.span: sentry_sdk.set_tag("timeout", "cache_timeout") From 5ebf7f92d4422e7c7185e9fbc3225e6d3081c462 Mon Sep 17 00:00:00 2001 From: Nikhar Saxena Date: Tue, 10 May 2022 09:47:25 -0700 Subject: [PATCH 4/4] Fix typing and add tests --- snuba/state/cache/redis/backend.py | 2 +- tests/state/test_cache.py | 33 +++++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/snuba/state/cache/redis/backend.py b/snuba/state/cache/redis/backend.py index 8bf5ade746..731283c62c 100644 --- a/snuba/state/cache/redis/backend.py +++ b/snuba/state/cache/redis/backend.py @@ -34,7 +34,7 @@ def __init__( prefix: str, codec: ExceptionAwareCodec[bytes, TValue], executor: ThreadPoolExecutor, - timeout_exception: Optional[Type[Exception]] = ExecutionTimeoutError, + timeout_exception: Type[Exception] = ExecutionTimeoutError, ) -> None: self.__client = client self.__prefix = prefix diff --git a/tests/state/test_cache.py b/tests/state/test_cache.py index 54fcd7194e..fb0d3a2659 100644 --- a/tests/state/test_cache.py +++ b/tests/state/test_cache.py @@ -12,7 +12,11 @@ import rapidjson from snuba.redis import RedisClientType, redis_client -from snuba.state.cache.abstract import Cache, ExecutionTimeoutError +from snuba.state.cache.abstract import ( + Cache, + ExecutionTimeoutError, + TigerExecutionTimeoutError, +) from snuba.state.cache.redis.backend import RedisCache from snuba.utils.codecs import ExceptionAwareCodec from snuba.utils.serializable_exception import SerializableException @@ -163,7 +167,30 @@ def worker() -> bytes: assert raised_exc -def test_get_readthrough_set_wait_timeout(backend: Cache[bytes]) -> None: +@pytest.mark.parametrize( + "backend, exc", + [ + pytest.param( + RedisCache(redis_client, "test", PassthroughCodec(), ThreadPoolExecutor()), + ExecutionTimeoutError, + id="regular cluster", + ), + pytest.param( + RedisCache( + redis_client, + "test", + PassthroughCodec(), + ThreadPoolExecutor(), + TigerExecutionTimeoutError, + ), + TigerExecutionTimeoutError, + id="tiger cluster", + ), + ], +) +def test_get_readthrough_set_wait_timeout( + backend: Cache[bytes], exc: Exception +) -> None: key = "key" value = b"value" @@ -186,7 +213,7 @@ def worker(timeout: int) -> bytes: with pytest.raises(TimeoutError): waiter_fast.result() - with pytest.raises(ExecutionTimeoutError): + with pytest.raises(exc): waiter_slow.result()