From dc3ac1948f831d5d3f84a0eba8506e1700ed9bc2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 2 Jun 2023 18:37:00 -0500 Subject: [PATCH 1/5] Some house-keeping --- synapse/handlers/federation.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 2eb28d55ac82..87b33b57cb7f 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -200,6 +200,7 @@ def __init__(self, hs: "HomeServer"): ) @trace + @tag_args async def maybe_backfill( self, room_id: str, current_depth: int, limit: int ) -> bool: @@ -214,6 +215,9 @@ async def maybe_backfill( limit: The number of events that the pagination request will return. This is used as part of the heuristic to decide if we should back paginate. + + Returns: + True if we actually tried to backfill something, otherwise False. """ # Starting the processing time here so we can include the room backfill # linearizer lock queue in the timing @@ -227,6 +231,8 @@ async def maybe_backfill( processing_start_time=processing_start_time, ) + @trace + @tag_args async def _maybe_backfill_inner( self, room_id: str, @@ -247,6 +253,9 @@ async def _maybe_backfill_inner( limit: The max number of events to request from the remote federated server. processing_start_time: The time when `maybe_backfill` started processing. Only used for timing. If `None`, no timing observation will be made. + + Returns: + True if we actually tried to backfill something, otherwise False. """ backwards_extremities = [ _BackfillPoint(event_id, depth, _BackfillPointType.BACKWARDS_EXTREMITY) @@ -310,7 +319,9 @@ async def _maybe_backfill_inner( logger.debug( "_maybe_backfill_inner: all backfill points are *after* current depth. Trying again with later backfill points." ) - return await self._maybe_backfill_inner( + run_as_background_process( + "_maybe_backfill_inner_anyway_with_max_depth", + self._maybe_backfill_inner, room_id=room_id, # We use `MAX_DEPTH` so that we find all backfill points next # time (all events are below the `MAX_DEPTH`) @@ -321,6 +332,10 @@ async def _maybe_backfill_inner( # overall otherwise the smaller one will throw off the results. processing_start_time=None, ) + # We return `False` because we're backfilling but doing it in the background + # and is just a best effort attempt for eventual consistency's sake. The + # return value of this function isn't used for anything yet anyway. + return False # Even after recursing with `MAX_DEPTH`, we didn't find any # backward extremities to backfill from. From 752bcd3072a774f567c8c805114be059a729b771 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 2 Jun 2023 19:18:10 -0500 Subject: [PATCH 2/5] Keep track of how many backfill points --- synapse/handlers/federation.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 87b33b57cb7f..da185e649da2 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -311,6 +311,14 @@ async def _maybe_backfill_inner( len(sorted_backfill_points), sorted_backfill_points, ) + set_tag( + SynapseTags.RESULT_PREFIX + "sorted_backfill_points", + str(sorted_backfill_points), + ) + set_tag( + SynapseTags.RESULT_PREFIX + "sorted_backfill_points.length", + str(len(sorted_backfill_points)), + ) # If we have no backfill points lower than the `current_depth` then # either we can a) bail or b) still attempt to backfill. We opt to try From dafd33374b103a0eb00c2f6a2053a5adba3cdbe4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 2 Jun 2023 19:25:47 -0500 Subject: [PATCH 3/5] Revert background change --- synapse/handlers/federation.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index da185e649da2..43bd6e7a2438 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -327,9 +327,7 @@ async def _maybe_backfill_inner( logger.debug( "_maybe_backfill_inner: all backfill points are *after* current depth. Trying again with later backfill points." ) - run_as_background_process( - "_maybe_backfill_inner_anyway_with_max_depth", - self._maybe_backfill_inner, + return await self._maybe_backfill_inner( room_id=room_id, # We use `MAX_DEPTH` so that we find all backfill points next # time (all events are below the `MAX_DEPTH`) @@ -340,11 +338,6 @@ async def _maybe_backfill_inner( # overall otherwise the smaller one will throw off the results. processing_start_time=None, ) - # We return `False` because we're backfilling but doing it in the background - # and is just a best effort attempt for eventual consistency's sake. The - # return value of this function isn't used for anything yet anyway. - return False - # Even after recursing with `MAX_DEPTH`, we didn't find any # backward extremities to backfill from. if not sorted_backfill_points: From 632b3a4b03d7299eeba6e6d526ec9216895c8d29 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 2 Jun 2023 19:26:14 -0500 Subject: [PATCH 4/5] Add whitespace --- synapse/handlers/federation.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index 43bd6e7a2438..57d6b70cff48 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -338,6 +338,7 @@ async def _maybe_backfill_inner( # overall otherwise the smaller one will throw off the results. processing_start_time=None, ) + # Even after recursing with `MAX_DEPTH`, we didn't find any # backward extremities to backfill from. if not sorted_backfill_points: From 51e7b057944fc606fa03fee8610745470dbc0cf6 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Fri, 2 Jun 2023 19:27:50 -0500 Subject: [PATCH 5/5] Add changelog --- changelog.d/15709.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/15709.misc diff --git a/changelog.d/15709.misc b/changelog.d/15709.misc new file mode 100644 index 000000000000..e9ce84a94021 --- /dev/null +++ b/changelog.d/15709.misc @@ -0,0 +1 @@ +Update docstring and traces on `maybe_backfill()` functions.