Implement Hi-Lo ID generation strategy in EntityManager #19
Conversation
…roved database efficiency
WalkthroughA Hi-Lo ID generation mechanism is added to the EntityManager with thread-safe per-field pools that pre-allocate ID ranges from a generator table, reducing database contention. Configuration options include a global default pool size and per-field customization via generator_pool_size attribute. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EntityManager
participant PoolState as Pool State<br/>(_hilo_pools)
participant GeneratorTable as Generator Table<br/>(Database)
Client->>EntityManager: Generate Entity ID
EntityManager->>PoolState: Check if pool exists & has IDs
alt Pool exists with available IDs
PoolState-->>EntityManager: Return next ID from pool
EntityManager-->>Client: Entity ID
else Pool exhausted or doesn't exist
EntityManager->>GeneratorTable: Atomically increment<br/>next_id by pool_size
GeneratorTable-->>EntityManager: Allocation range<br/>(low, high)
EntityManager->>PoolState: Store new pool range
PoolState-->>EntityManager: ✓ Pool initialized
EntityManager->>PoolState: Grab first ID from pool
PoolState-->>EntityManager: Return ID
EntityManager-->>Client: Entity ID
end
note over EntityManager,PoolState: All pool operations<br/>protected by _hilo_lock
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
| ### Changed | ||
|
|
||
| * | ||
|
|
||
| ### Fixed | ||
|
|
||
| * |
There was a problem hiding this comment.
Remove empty placeholder bullets in Changed and Fixed sections.
The placeholder bullet points (*) on lines 19 and 23 are incomplete and don't follow Keep a Changelog conventions. Either populate these sections with actual changes/fixes or remove them entirely if there are no entries for this PR.
🔎 Suggested fix
If there are no actual changes or fixes in this PR beyond the Added items, remove both sections:
### Added
* Hi-Lo generator strategy (`generator_type="hilo"`) for entity ID generation that reduces database contention by pre-allocating ID pools
* New `HILO_POOL_SIZE` constant (default: 100) for configuring pool size globally
* Support for per-field pool size customization via `generator_pool_size` attribute
* Thread-safe pool management with local locking that only accesses the database during pool allocation
-
-### Changed
-
-*
-
-### Fixed
-
-*Alternatively, if there are additional changes or fixes to document, populate these sections with the corresponding entries.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 17 to 23, the "Changed" and "Fixed" sections
contain empty placeholder bullets (*) which should be removed or replaced;
either delete the empty bullet lines (or the entire section headings if there
are no entries to report) or replace each placeholder with the actual change/fix
text following Keep a Changelog conventions so the file contains only meaningful
entries.
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) |
| cursor.close() | ||
|
|
||
| # checks if a row exists for this field name | ||
| if not rows or rows[0] == None: |
There was a problem hiding this comment.
Testing for None should use the 'is' operator.
| if not rows or rows[0] == None: | |
| if not rows or rows[0] is None: |
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 efficiencySummary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.