Implement Hi-Lo ID generation strategy in EntityManager - #19
Conversation
📝 WalkthroughWalkthroughEntityManager now supports Hi-Lo identifier generation with configurable per-field pools, thread-safe in-memory allocation, transactional range reservation, rollback discard, and pool reset. Tests, changelog entries, and design documentation cover the new strategy. ChangesHi-Lo ID generation
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
data/src/entity_manager/system.py (2)
2881-2915: Validategenerator_pool_sizeto avoid misconfiguration edge cases
_generate_hilo()pullsgenerator_pool_sizedirectly from the field metadata with no validation. If someone accidentally setsgenerator_pool_sizeto0or a negative/non‑numeric value, the arithmetic in_hilo_allocate_pool/_hilo_atomic_incrementcan yield an empty or inverted range, causing repeated DB pool allocations and duplicated IDs for that field.Given this is developer-provided configuration, it’s low-risk but worth guarding.
Proposed validation inside
_generate_hilodef _generate_hilo(self, entity, name): @@ - field_name = value.get("generator_field_name", field_name) - pool_size = value.get("generator_pool_size", HILO_POOL_SIZE) + field_name = value.get("generator_field_name", field_name) + pool_size = value.get("generator_pool_size", HILO_POOL_SIZE) + + # normalize and validate pool size to avoid misconfiguration + try: + pool_size = int(pool_size) + except (TypeError, ValueError): + raise exceptions.RuntimeError( + "generator_pool_size for %s must be an integer" % field_name + ) + if pool_size <= 0: + raise exceptions.RuntimeError( + "generator_pool_size for %s must be a positive integer" % field_name + )
2917-3063: Hi-Lo pool logic, concurrency, and SQL constructionThe Hi-Lo helpers are generally solid and align with the existing table-based generator semantics:
_hilo_grab_idcorrectly:
- Uses a single per‑EntityManager lock (
_hilo_lock) to make pool allocation and consumption atomic across threads in this process.- Forces an allocation on first use via the synthetic
(current_id=1, max_id=0)state.- Maintains the invariant that pools are
[low, high]and allocates a new pool whencurrent_id > max_id.
_hilo_allocate_pool/_hilo_atomic_increment:
- Ensure the generator table exists via
create_generator().- Use
lock_table(..., {"field_name": "name", "field_value": "'" + name + "'"})plus aselect ... for update-style query, matching the pattern used by_next_id_query/_increment_id_query.- Keep
next_idas “first free ID after our reserved range”, so the allocated [low, high] ranges are contiguous and non-overlapping even with mixed pool sizes or legacygenerator_type="table"usage.On the static-analysis side:
- The new string-built queries in
_hilo_atomic_incrementfollow the same pattern as the pre-existing generator code and use_escape_text(field_name)before interpolation, so they don’t introduce a new SQL injection surface beyond what’s already present. The S608 warnings from static analysis are expected in this codebase and can be safely ignored here.if not rows or rows[0] == None:matches the style elsewhere in this module; switching tois Nonewould be more idiomatic but is purely stylistic (E711) and not required.Overall, the Hi-Lo pool acquisition and concurrency behavior look correct and compatible with the existing generator table design.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)data/src/entity_manager/system.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
data/src/entity_manager/system.py (3)
data/src/entity_mysql/system.py (4)
lock_table(353-355)cursor(1022-1024)execute_query(377-552)close(971-987)data/src/entity_pgsql/system.py (4)
lock_table(305-307)cursor(809-811)execute_query(327-421)close(785-792)data/src/entity_manager/decorators.py (1)
lock_table(121-194)
🪛 Ruff (0.14.8)
data/src/entity_manager/system.py
353-353: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
3030-3030: Possible SQL injection vector through string-based query construction
(S608)
3039-3039: Comparison to None should be cond is None
Replace with cond is None
(E711)
3056-3061: Possible SQL injection vector through string-based query construction
(S608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (3)
CHANGELOG.md (1)
1-16: LGTM! Format and content are well-documented.The CHANGELOG properly follows Keep a Changelog conventions and accurately documents the Hi-Lo generator features introduced in this PR. The Added section comprehensively covers the new functionality, configuration options, and thread-safety guarantees.
data/src/entity_manager/system.py (2)
40-40: Hi-Lo import and default pool size look goodThe
threadingimport andHILO_POOL_SIZE = 100default, with a clear docstring and per-field override viagenerator_pool_size, are consistent and sufficient. No changes needed here.Also applies to: 66-70
353-360: Hi-Lo pool state and locking are correctly scoped per EntityManagerUsing
_hilo_poolsand_hilo_lockas instance-level state initialized in__init__avoids cross-instance interference while still sharing pools across threads for a given manager. This mirrors how other mutable attributes (like_exists) are handled in this class, so the pattern is consistent with existing design.Also applies to: 394-396
There was a problem hiding this comment.
Pull request overview
This pull request implements a Hi-Lo ID generation strategy for the EntityManager to reduce database contention during entity ID generation. The Hi-Lo strategy pre-allocates pools of IDs, minimizing database round trips by only accessing the database when a pool is exhausted rather than for every single ID.
Key Changes:
- Added Hi-Lo ID generator implementation with configurable pool sizes (global and per-field)
- Implemented thread-safe pool management using threading locks and per-field pool state
- Introduced CHANGELOG.md to document the new feature
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| data/src/entity_manager/system.py | Implements the Hi-Lo ID generation strategy with _generate_hilo, _hilo_grab_id, _hilo_allocate_pool, and _hilo_atomic_increment methods; adds threading support and pool management structures |
| CHANGELOG.md | Documents the addition of the Hi-Lo generator feature and related configuration options |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _generate_hilo(self, entity, name): | ||
| # retrieves the (entity) class associated with | ||
| # the entity to generate the value | ||
| entity_class = entity.__class__ | ||
|
|
||
| # uses the name to retrieve the value (map) | ||
| # containing the definition of the attribute | ||
| value = getattr(entity_class, name) | ||
|
|
||
| # retrieves the map containing the various entity names | ||
| # associated with their respective classes and then | ||
| # retrieves the entity class that "owns" the value | ||
| names_map = entity_class.get_names_map() | ||
| name_class = names_map[name] | ||
|
|
||
| # retrieves the name of the table associated with the | ||
| # current name and uses it to create the default field | ||
| # name for the generation table (class name and field name) | ||
| table_name = name_class.get_name() | ||
| field_name = "%s_%s" % (table_name, name) | ||
|
|
||
| # tries to retrieve the (generator) field name defaulting | ||
| # to the name of the default field name, the pool size can | ||
| # also be customized per field via the generator_pool_size | ||
| field_name = value.get("generator_field_name", field_name) | ||
| pool_size = value.get("generator_pool_size", HILO_POOL_SIZE) | ||
|
|
||
| # grabs an id value using the Hi-Lo pool allocation strategy | ||
| # which reduces database contention by pre-allocating ranges | ||
| value = self._hilo_grab_id(field_name, pool_size) | ||
| value = int(value) | ||
|
|
||
| # sets the generated value in the entity, final setting | ||
| # of the generated value | ||
| entity.set_value(name, value) | ||
|
|
||
| def _hilo_grab_id(self, field_name, pool_size): | ||
| """ | ||
| Retrieves the next available ID using the Hi-Lo pool | ||
| allocation strategy, allocating a new pool from the | ||
| database when the current pool is exhausted. | ||
|
|
||
| This method significantly reduces database contention | ||
| by acquiring pool_size IDs per database access instead | ||
| of one ID per access (as the table generator does). | ||
|
|
||
| :type field_name: String | ||
| :param field_name: The name of the field for which to | ||
| retrieve the next ID value. | ||
| :type pool_size: int | ||
| :param pool_size: The number of IDs to pre-allocate | ||
| when the pool is exhausted. | ||
| :rtype: int | ||
| :return: The next available ID for the field. | ||
| """ | ||
|
|
||
| # acquires the lock to ensure thread-safe access to the | ||
| # Hi-Lo pools, this is a local lock that does not involve | ||
| # any database locking during normal ID consumption | ||
| with self._hilo_lock: | ||
| # checks if a pool exists for this field and if so | ||
| # retrieves the current state of the pool | ||
| if field_name in self._hilo_pools: | ||
| current_id, max_id = self._hilo_pools[field_name] | ||
| else: | ||
| # no pool exists, force allocation by setting | ||
| # current above max | ||
| current_id = 1 | ||
| max_id = 0 | ||
|
|
||
| # if the current id exceeds the max id, the pool is | ||
| # exhausted and a new pool must be allocated from | ||
| # the database | ||
| if current_id > max_id: | ||
| self._hilo_allocate_pool(field_name, pool_size) | ||
| current_id, max_id = self._hilo_pools[field_name] | ||
|
|
||
| # consume one ID from the pool and update the pool state | ||
| self._hilo_pools[field_name] = (current_id + 1, max_id) | ||
| return current_id | ||
|
|
||
| def _hilo_allocate_pool(self, field_name, pool_size): | ||
| """ | ||
| Allocates a new pool of IDs from the database by atomically | ||
| incrementing the generator counter by pool_size. | ||
|
|
||
| The pool is stored as (low, high) where low is the first | ||
| ID to use and high is the last ID in the pool range. | ||
|
|
||
| :type field_name: String | ||
| :param field_name: The name of the field for which to | ||
| allocate a new pool. | ||
| :type pool_size: int | ||
| :param pool_size: The number of IDs to allocate in | ||
| the new pool. | ||
| """ | ||
|
|
||
| # ensures the generator table exists before attempting | ||
| # to allocate a pool | ||
| self.create_generator() | ||
|
|
||
| # atomically increment the generator counter by pool_size | ||
| # and retrieve the new high value | ||
| new_high = self._hilo_atomic_increment(field_name, pool_size) | ||
|
|
||
| # our allocated range is [new_high - pool_size, new_high - 1] | ||
| # since new_high is the next available value after our range | ||
| low = new_high - pool_size | ||
| high = new_high - 1 | ||
|
|
||
| # store the pool state as (current_id, max_id) | ||
| self._hilo_pools[field_name] = (low, high) | ||
|
|
||
| def _hilo_atomic_increment(self, field_name, increment): | ||
| """ | ||
| Atomically increments the generator counter for the given | ||
| field by the specified increment amount and returns the | ||
| new `next_id` value. | ||
|
|
||
| This method uses database locking to ensure atomicity | ||
| across concurrent processes/threads, but the lock is only | ||
| held for the duration of the pool allocation, not for each | ||
| individual ID. | ||
|
|
||
| :type field_name: String | ||
| :param field_name: The name of the field for which to | ||
| increment the generator counter. | ||
| :type increment: int | ||
| :param increment: The amount by which to increment the | ||
| counter (typically the pool size). | ||
| :rtype: int | ||
| :return: The new `next_id` value after incrementing. | ||
| """ | ||
|
|
||
| # escapes the field name value to avoid possible | ||
| # security problems (SQL injection) | ||
| name = self._escape_text(field_name) | ||
|
|
||
| # retrieves the current modification time for | ||
| # the generator as the current system time | ||
| _mtime = time.time() | ||
|
|
||
| # locks the generator table row and retrieves the current | ||
| # next_id value, this uses SELECT ... FOR UPDATE on MySQL | ||
| # or equivalent locking on other engines | ||
| self.lock_table( | ||
| GENERATOR_VALUE, {"field_name": "name", "field_value": "'" + name + "'"} | ||
| ) | ||
| query = ( | ||
| "select name, next_id from " + GENERATOR_VALUE + " where name = '%s'" % name | ||
| ) | ||
| cursor = self.execute_query(query, False) | ||
| try: | ||
| rows = [value[1] for value in cursor] | ||
| finally: | ||
| cursor.close() | ||
|
|
||
| # checks if a row exists for this field name | ||
| if not rows or rows[0] == None: | ||
| # first allocation for this field, start at 1 and | ||
| # reserve IDs up to 1 + increment (so next available | ||
| # after our pool is 1 + increment) | ||
| next_id = 1 + increment | ||
| query = "insert into %s(name, next_id, _mtime) values('%s', %d, %f)" % ( | ||
| GENERATOR_VALUE, | ||
| name, | ||
| next_id, | ||
| _mtime, | ||
| ) | ||
| self.execute_query(query) | ||
| return next_id | ||
| else: | ||
| # increment the existing counter by the pool size | ||
| current_next = rows[0] | ||
| new_next = current_next + increment | ||
| query = "update %s set next_id = %d, _mtime = %f where name = '%s'" % ( | ||
| GENERATOR_VALUE, | ||
| new_next, | ||
| _mtime, | ||
| name, | ||
| ) | ||
| self.execute_query(query) | ||
| return new_next | ||
|
|
There was a problem hiding this comment.
The new Hi-Lo generator strategy lacks test coverage. Since the repository has comprehensive tests for the entity manager (see data/src/entity_manager/test.py with test_generate_id), the new _generate_hilo, _hilo_grab_id, _hilo_allocate_pool, and _hilo_atomic_increment methods should have corresponding test cases to verify: 1) correct ID generation and incrementing, 2) pool allocation and exhaustion behavior, 3) thread-safety with concurrent ID requests, and 4) correct handling of the first allocation vs subsequent allocations.
| value = getattr(entity_class, name) | ||
|
|
||
| # retrieves the map containing the various entity names | ||
| # associated with their respective classes and then | ||
| # retrieves the entity class that "owns" the value | ||
| names_map = entity_class.get_names_map() | ||
| name_class = names_map[name] | ||
|
|
||
| # retrieves the name of the table associated with the | ||
| # current name and uses it to create the default field | ||
| # name for the generation table (class name and field name) | ||
| table_name = name_class.get_name() | ||
| field_name = "%s_%s" % (table_name, name) | ||
|
|
||
| # tries to retrieve the (generator) field name defaulting | ||
| # to the name of the default field name, the pool size can | ||
| # also be customized per field via the generator_pool_size | ||
| field_name = value.get("generator_field_name", field_name) | ||
| pool_size = value.get("generator_pool_size", HILO_POOL_SIZE) | ||
|
|
||
| # grabs an id value using the Hi-Lo pool allocation strategy | ||
| # which reduces database contention by pre-allocating ranges | ||
| value = self._hilo_grab_id(field_name, pool_size) |
There was a problem hiding this comment.
The variable value is reused with different meanings (lines 2888 and 2910), which reduces code clarity. On line 2888, value holds the attribute definition map, but on line 2910 it's reassigned to hold the generated ID. Consider using a more descriptive name like attr_definition for the first usage to distinguish it from the generated ID value.
| # grabs an id value using the Hi-Lo pool allocation strategy | ||
| # which reduces database contention by pre-allocating ranges | ||
| value = self._hilo_grab_id(field_name, pool_size) | ||
| value = int(value) |
There was a problem hiding this comment.
The int(value) conversion on line 2911 is redundant since _hilo_grab_id already returns an integer value, as documented in its return type (line 2933-2934). This conversion can be removed for cleaner code.
| value = int(value) |
| _hilo_pools = {} | ||
| """ Map associating field names with their respective Hi-Lo pool | ||
| state as a tuple of (current_id, max_id) for the pool range """ | ||
|
|
||
| _hilo_lock = None | ||
| """ Lock for thread-safe access to the Hi-Lo pools, ensures | ||
| that pool allocation and ID consumption are atomic """ | ||
|
|
There was a problem hiding this comment.
Defining _hilo_pools and _hilo_lock as class-level attributes (lines 353, 357) and then reassigning them in __init__ (lines 394-395) is problematic. Class-level mutable attributes are shared across all instances of the class, which could cause thread-safety issues if multiple EntityManager instances exist. The class-level declarations should be removed, and these attributes should only be initialized as instance attributes in __init__.
|
|
||
| ### Changed | ||
|
|
||
| * | ||
|
|
||
| ### Fixed | ||
|
|
||
| * |
There was a problem hiding this comment.
The "Changed" and "Fixed" sections contain only asterisks with no content. These placeholder sections should either be removed or omitted until there is actual content to document. Empty sections with just asterisks can be confusing in a changelog.
| ### Changed | |
| * | |
| ### Fixed | |
| * |
| # acquires the lock to ensure thread-safe access to the | ||
| # Hi-Lo pools, this is a local lock that does not involve | ||
| # any database locking during normal ID consumption | ||
| with self._hilo_lock: | ||
| # checks if a pool exists for this field and if so | ||
| # retrieves the current state of the pool | ||
| if field_name in self._hilo_pools: | ||
| current_id, max_id = self._hilo_pools[field_name] | ||
| else: | ||
| # no pool exists, force allocation by setting | ||
| # current above max | ||
| current_id = 1 | ||
| max_id = 0 | ||
|
|
||
| # if the current id exceeds the max id, the pool is | ||
| # exhausted and a new pool must be allocated from | ||
| # the database | ||
| if current_id > max_id: | ||
| self._hilo_allocate_pool(field_name, pool_size) | ||
| current_id, max_id = self._hilo_pools[field_name] | ||
|
|
||
| # consume one ID from the pool and update the pool state | ||
| self._hilo_pools[field_name] = (current_id + 1, max_id) | ||
| return current_id |
There was a problem hiding this comment.
The lock is held during the entire database operation in _hilo_allocate_pool (called at line 2955 while holding _hilo_lock acquired at line 2940). This means all ID generation across all fields is blocked when any single field needs to allocate a new pool. For better performance, consider using a per-field lock or releasing the global lock before the database call and re-acquiring it after. A per-field lock would allow concurrent pool allocations for different fields.
| # acquires the lock to ensure thread-safe access to the | |
| # Hi-Lo pools, this is a local lock that does not involve | |
| # any database locking during normal ID consumption | |
| with self._hilo_lock: | |
| # checks if a pool exists for this field and if so | |
| # retrieves the current state of the pool | |
| if field_name in self._hilo_pools: | |
| current_id, max_id = self._hilo_pools[field_name] | |
| else: | |
| # no pool exists, force allocation by setting | |
| # current above max | |
| current_id = 1 | |
| max_id = 0 | |
| # if the current id exceeds the max id, the pool is | |
| # exhausted and a new pool must be allocated from | |
| # the database | |
| if current_id > max_id: | |
| self._hilo_allocate_pool(field_name, pool_size) | |
| current_id, max_id = self._hilo_pools[field_name] | |
| # consume one ID from the pool and update the pool state | |
| self._hilo_pools[field_name] = (current_id + 1, max_id) | |
| return current_id | |
| # loop until an ID is successfully obtained; this allows us | |
| # to release the global lock while performing database work | |
| # and then retry the acquisition | |
| while True: | |
| # acquires the lock to ensure thread-safe access to the | |
| # Hi-Lo pools; this lock is held only while accessing | |
| # in-memory state and not during database operations | |
| self._hilo_lock.acquire() | |
| try: | |
| # checks if a pool exists for this field and if so | |
| # retrieves the current state of the pool | |
| if field_name in self._hilo_pools: | |
| current_id, max_id = self._hilo_pools[field_name] | |
| else: | |
| # no pool exists, force allocation by setting | |
| # current above max | |
| current_id = 1 | |
| max_id = 0 | |
| # if the current id exceeds the max id, the pool is | |
| # exhausted and a new pool must be allocated from | |
| # the database; release the lock before doing so | |
| if current_id > max_id: | |
| # do not update the pool here; allocation will | |
| # happen outside the lock, then we will retry | |
| pass | |
| else: | |
| # consume one ID from the pool and update the pool state | |
| self._hilo_pools[field_name] = (current_id + 1, max_id) | |
| return current_id | |
| finally: | |
| self._hilo_lock.release() | |
| # pool was exhausted; allocate a new pool from the database | |
| # outside the global lock to avoid blocking other fields' | |
| # ID generation while the database operation runs | |
| self._hilo_allocate_pool(field_name, pool_size) |
…roved database efficiency
- Generalize increment_id and _increment_id_query with an increment parameter (default 1) - Remove the duplicated _hilo_atomic_increment method in favor of increment_id - Keep the exact same locking and query semantics for both generator strategies
- Discard the field pool when the allocating transaction is rolled back, avoiding duplicated identifiers from ranges whose reservation was undone - Reset the Hi-Lo pools state when the data source is destroyed
- New Ticket mock entity using the Hi-Lo generator with a custom pool size - Tests for grab_id, increment_id and the increment parameter - Tests for Hi-Lo pool consumption, allocation, discard on rollback and reset
- New COP-002 design document describing the problem, solution, trade-offs and usage of the Hi-Lo strategy - Includes benchmark results comparing the table and Hi-Lo generation strategies - Register the new document in the design documents index
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Rebased onto master and reworked the implementation:
Full test suite passes (115 tests). |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
data/src/entity_manager/test.py (1)
1848-1854: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover rollback after another transaction consumes the pool.
Add a threaded regression where transaction A allocates the pool, transaction B consumes and commits an ID, then transaction A rolls back. The next reservation must not reproduce B's committed ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/src/entity_manager/test.py` around lines 1848 - 1854, Extend the rollback coverage around _hilo_allocate_pool and rollback with a threaded regression: have transaction A reserve the pool, transaction B consume and commit an ID from it, then roll back transaction A. Verify the next reservation does not reproduce B’s committed ID, while preserving the existing pool-discard assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@data/src/entity_manager/system.py`:
- Around line 1964-1965: Update increment_id to validate increment before
calling _increment_id_query: require a positive integer and reject zero,
negative, fractional, and other non-integral values with the module’s
established validation/error behavior. Only construct the query after validation
succeeds, preserving normal ID allocation for valid increments.
- Around line 3029-3034: Prevent Hi-Lo pools allocated by one transaction from
becoming globally reusable before that transaction commits: update the
reservation/publication flow around _hilo_discard_pool and the surrounding
transaction handling in data/src/entity_manager/system.py:3029-3034 so pools are
reserved independently or remain transaction-local until commit, while
preserving rollback cleanup. Add a threaded regression test in
data/src/entity_manager/test.py:1848-1854 where transaction A allocates,
transaction B commits IDs, and A rolls back; assert that no committed ID is
reissued.
---
Nitpick comments:
In `@data/src/entity_manager/test.py`:
- Around line 1848-1854: Extend the rollback coverage around _hilo_allocate_pool
and rollback with a threaded regression: have transaction A reserve the pool,
transaction B consume and commit an ID from it, then roll back transaction A.
Verify the next reservation does not reproduce B’s committed ID, while
preserving the existing pool-discard assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98a22cce-bc1a-4407-afce-81dcda1d33fe
📒 Files selected for processing (6)
CHANGELOG.mddata/src/entity_manager/mocks.pydata/src/entity_manager/system.pydata/src/entity_manager/test.pydoc/design/002-hilo_id_generation.mddoc/design/README.md
| def increment_id(self, name, increment=1): | ||
| query, next_id = self._increment_id_query(name, increment) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject non-positive and non-integral increments.
increment <= 0 stalls or reverses the persisted counter, while fractional values can be truncated by SQL formatting. Since generator_pool_size reaches this API, invalid field configuration can produce overlapping ID ranges. Validate a positive integer before constructing the query.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/src/entity_manager/system.py` around lines 1964 - 1965, Update
increment_id to validate increment before calling _increment_id_query: require a
positive integer and reject zero, negative, fractional, and other non-integral
values with the module’s established validation/error behavior. Only construct
the query after validation succeeds, preserving normal ID allocation for valid
increments.
| # in case a transaction is currently open the pool must be | ||
| # discarded once the transaction is "rollbacked" as the | ||
| # reservation of the range in the data source is going to | ||
| # be undone (would generate duplicated identifiers) | ||
| if self.has_transaction(): | ||
| self.after_rollback(lambda: self._hilo_discard_pool(field_name)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Prevent cross-transaction reuse of rolled-back Hi-Lo ranges.
A pool reserved inside transaction A is published globally before commit, allowing transaction B to commit IDs from a reservation that A can later roll back.
data/src/entity_manager/system.py#L3029-L3034: reserve independently, or keep the pool transaction-local until commit before publishing it.data/src/entity_manager/test.py#L1848-L1854: add a threaded A-allocates/B-commits/A-rolls-back regression test and assert no committed ID is reissued.
📍 Affects 2 files
data/src/entity_manager/system.py#L3029-L3034(this comment)data/src/entity_manager/test.py#L1848-L1854
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data/src/entity_manager/system.py` around lines 3029 - 3034, Prevent Hi-Lo
pools allocated by one transaction from becoming globally reusable before that
transaction commits: update the reservation/publication flow around
_hilo_discard_pool and the surrounding transaction handling in
data/src/entity_manager/system.py:3029-3034 so pools are reserved independently
or remain transaction-local until commit, while preserving rollback cleanup. Add
a threaded regression test in data/src/entity_manager/test.py:1848-1854 where
transaction A allocates, transaction B commits IDs, and A rolls back; assert
that no committed ID is reissued.
This pull request introduces a new Hi-Lo ID generator strategy to the entity management system, aimed at reducing database contention and improving performance when generating entity IDs. The Hi-Lo strategy pre-allocates pools of IDs, supports global and per-field pool size configuration, and ensures thread-safe pool management with minimal locking. The changes are documented in a new
CHANGELOG.mdfile.Hi-Lo ID Generation Strategy:
generator_type="hilo") for entity ID generation, which reduces database contention by pre-allocating pools of IDs and only accessing the database when a pool is exhausted.HILO_POOL_SIZEconstant (default: 100) for global pool size configuration, and support for per-field pool size customization via thegenerator_pool_sizeattribute. [1] [2]_hilo_lock) and a per-field pool state (_hilo_pools) to ensure atomic pool allocation and ID consumption. [1] [2] [3]Documentation:
CHANGELOG.mdfile following Keep a Changelog and Semantic Versioning guidelines, documenting the introduction of the Hi-Lo generator and related features.Dependencies:
threadingmodule to support thread-safe operations in the Hi-Lo generator.…roved database efficiency