Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

List of the most important changes for each release.

## 0.8.15
- Improves buffer serialization performance by performing bulk counter lookup

## 0.8.14
- Adds utility for addressing immediate FK constraints caused by Django upgrade, automatically performed for morango models in a Django migration.
- Adds retry behavior for low-level connection issues not handled by `urllib3` retries
Expand Down
2 changes: 1 addition & 1 deletion morango/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = "0.8.14"
__version__ = "0.8.15"
29 changes: 29 additions & 0 deletions morango/api/serializers.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
from collections import defaultdict

from rest_framework import exceptions
from rest_framework import serializers
from rest_framework.fields import ReadOnlyField
Expand Down Expand Up @@ -158,11 +160,38 @@ class Meta:
read_only_fields = fields


class BufferListSerializer(serializers.ListSerializer):
Comment thread
bjester marked this conversation as resolved.
def to_representation(self, data):
buffers = list(data)
if buffers:
rmcb_map = defaultdict(list)
transfer_session_ids = {b.transfer_session_id for b in buffers}
# Morango implementation will only ever call this for one transfer session ID at a time,
# but a loop makes the code straightforward regardless and limits the quantity of
# SQLite variables in use
for transfer_session_id in transfer_session_ids:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This loop is written to handle multiple transfer_session_ids in one call (per the comment above), which is exactly the shape where the previously-fixed scoping bug lived. The new regression test only covers a single transfer session, so a future scoping regression on this branch wouldn't be caught. If this path is kept as future-proofing, add a multi-session test; otherwise consider simplifying to the single-session case that's actually exercised.

# bulk-fetch all RMCB records needed for this batch of buffers in a
# single query, then cache the relevant subset on each buffer, instead
# of letting each buffer's nested rmcb_list serializer issue its own query
rmcb_queryset = RecordMaxCounterBuffer.objects.filter(
transfer_session_id=transfer_session_id,
model_uuid__in={b.model_uuid for b in buffers if b.transfer_session_id == transfer_session_id},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

important: This IN clause is sized to the number of buffers in the current page, and page size is driven by the caller-configurable sync chunk_size (NetworkSyncConnection.chunk_size, default 500 but overridable via create_sync_session/resume_sync_session). Since the codebase's only tested backend is SQLite, a caller that sets chunk_size above SQLite's compiled SQLITE_MAX_VARIABLE_NUMBER (999 on many stock builds) would hit OperationalError: too many SQL variables here — a failure mode that didn't exist when each buffer issued its own single-value filter. Consider sub-chunking the IN clause independent of chunk_size, or documenting/enforcing a max safe chunk_size.

)
for rmcb in rmcb_queryset:
rmcb_map[(transfer_session_id, rmcb.model_uuid)].append(rmcb)
for buffer in buffers:
buffer._rmcb_list = rmcb_map[
(buffer.transfer_session_id, buffer.model_uuid)
]
return super(BufferListSerializer, self).to_representation(buffers)


class BufferSerializer(serializers.ModelSerializer):
rmcb_list = RecordMaxCounterBufferSerializer(many=True)

class Meta:
model = Buffer
list_serializer_class = BufferListSerializer
fields = (
"serialized",
"deleted",
Expand Down
5 changes: 5 additions & 0 deletions morango/models/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,11 @@ class Meta:
unique_together = ("transfer_session", "model_uuid")

def rmcb_list(self):
# allow callers (e.g. BufferListSerializer) to batch-fetch RMCB records
# for many buffers at once and cache them here, to avoid an N+1 query
# pattern when serializing a large number of buffers
if hasattr(self, "_rmcb_list"):
return self._rmcb_list
return RecordMaxCounterBuffer.objects.filter(
model_uuid=self.model_uuid, transfer_session_id=self.transfer_session_id
)
Expand Down
16 changes: 16 additions & 0 deletions tests/testapp/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1073,6 +1073,22 @@ def test_buffer_serializer_makes_no_transfer_session_query(self):
for q in ctx.captured_queries:
self.assertFalse('morango_transfersession' in q['sql'])

def test_buffer_serializer_batches_rmcb_queries_for_many(self):
transfer_session_id = self.create_records_for_pulling(count=10)
buffers = Buffer.objects.filter(
transfer_session_id=transfer_session_id
).order_by("pk")

with CaptureQueriesContext(connection) as ctx:
BufferSerializer(buffers, many=True).data

rmcb_queries = [
q
for q in ctx.captured_queries
if "recordmaxcounterbuffer" in q["sql"].lower()
]
self.assertEqual(len(rmcb_queries), 1)

def test_pull_valid_buffer_list(self):

transfer_session_id = self.create_records_for_pulling()
Expand Down
Loading