Skip to content

Commit

Permalink
fix: Excessive gap locking from hash naming (frappe#28349)
Browse files Browse the repository at this point in the history
Because of large common prefix hash naming becomes "too sequential" when
doing a lot of concurrent writes.

I don't know a good tradeoff between both use cases:
1. Lots of reads - prefers large shared prefix.
2. Lots of writes - prefers small shared prefix.

But as of now this punishes writes too badly in form of excessive
locking. Until a better fix is found, it's better to keep it prefix free.

---

A better fix would be a tradeoff of between these two:

1. Reads - temporal locality should result in spatial locality on disk.
2. Writes - temporal locality should NOT result in spatial locality.

temporal locality = data inserted around same time
spatial locality = data sits next to each other in DB pages.

This can be achieved by adding a small request/job specific part to
prefix so each concurrent request has it's own different locality when
writing data.
  • Loading branch information
ankush authored Nov 1, 2024
1 parent fcae605 commit 16407a5
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
3 changes: 1 addition & 2 deletions frappe/model/naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,7 @@ def make_autoname(key="", doctype="", doc="", *, ignore_validate=False):
DE/09/01/00001 where 09 is the year, 01 is the month and 00001 is the series
"""
if key == "hash":
# Makeshift "ULID": first 4 chars are based on timestamp, other 6 are random
return _get_timestamp_prefix() + _generate_random_string(6)
return _generate_random_string(10)

series = NamingSeries(key)
return series.generate_next_name(doc, ignore_validate=ignore_validate)
Expand Down
2 changes: 2 additions & 0 deletions frappe/tests/test_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# License: MIT. See LICENSE

import time
import unittest
from uuid import UUID

import uuid_utils
Expand Down Expand Up @@ -407,6 +408,7 @@ def test_custom_parser(self):
expected_name = "TODO-" + nowdate().split("-")[1] + "-" + "0001"
self.assertEqual(name, expected_name)

@unittest.skip("This is not supported anymore, see #28349.")
@retry(
retry=retry_if_exception_type(AssertionError),
stop=stop_after_attempt(3),
Expand Down

0 comments on commit 16407a5

Please sign in to comment.