Read only uses #9237
Read only uses #9237
Conversation
WalkthroughThe changes introduce preferred pool selection for SQL query execution, allowing read operations to route to read-only replicas. Three files were modified: the TypeScript interface adds a Changes
Sequence DiagramsequenceDiagram
participant Caller
participant MDStore
participant PostgresTable
participant PoolMgr as Pool Manager
Caller->>MDStore: Read operation (e.g., find_unreclaimed_objects)
MDStore->>PostgresTable: executeSQL(query, params, {preferred_pool: 'read_only'})
PostgresTable->>PoolMgr: get_pool('read_only')
alt Pool found
PoolMgr-->>PostgresTable: read_only pool
else Pool not found
PoolMgr->>PoolMgr: Fallback to default pool
PoolMgr-->>PostgresTable: default pool
end
PostgresTable->>PostgresTable: _do_query(pool, query, params)
PostgresTable-->>MDStore: query result
MDStore-->>Caller: operation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The changes involve consistent pattern application across multiple files with new fallback logic in pool selection, but follow a straightforward parameter-passing mechanism without complex interdependencies. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Signed-off-by: Amit Prinz Setter <[email protected]>
Signed-off-by: Amit Prinz Setter <[email protected]>
Signed-off-by: Amit Prinz Setter <[email protected]>
alphaprinz
force-pushed
the
read_only_uses
branch
from
0a008b6 to
d699655
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/util/postgres_client.js (1)
632-642: Review the fallback behavior in get_pool.The fallback logic on lines 636-637 silently falls back to the default pool when a requested pool key is invalid:
if (key && key !== this.pool_key) { return this.get_pool(); }Concerns:
- Silent fallback masks configuration issues: If code requests a non-existent pool (e.g., typo like
'read_onyl'), it silently uses the default pool instead of failing fast.- Misleading error message: After the fallback attempt, line 639 throws an error mentioning the requested
key, but the fallback might have already succeeded withthis.pool_key.Recommendation:
Consider removing the fallback or adding a warning log when falling back, so configuration issues are visible:get_pool(key = this.pool_key) { const pool = this.client.get_pool(key); if (!pool) { - //if original get_pool was no for the default this.pool_key, try also this.pool_key + // If original get_pool was not for the default this.pool_key, try also this.pool_key if (key && key !== this.pool_key) { + dbg.warn(`Pool ${key} not found, falling back to default pool ${this.pool_key}`); return this.get_pool(); } throw new Error(`The postgres clients pool ${key} disconnected`); } return pool; }Alternatively, remove the fallback entirely to fail fast on invalid pool names.
src/server/object_services/md_store.js (1)
814-835: Route mapReduce operations to read-only pool for consistent replica distribution.The
list_objectsandlist_object_versionsmethods usemapReducefor queries with delimiters (lines 814-824 and 864-874), but these calls don't passpreferred_poolto route to the read-only pool. While thefind()operations support routing viapreferred_pool: 'read_only'(as seen elsewhere in md_store.js), the mapReduce calls should be consistent.To fix:
- Update
mapReduceListObjects()andmapReduceAggregate()in postgres_client.js to extractoptions.preferred_pooland pass it tothis.single_query()(currently they callthis.single_query(mr_q)without the pool parameter)- Pass
preferred_pool: 'read_only'in the options object when callingthis._objects.mapReduce()at lines 814-824 and 864-874 in md_store.js
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sdk/nb.d.ts(1 hunks)src/server/bg_services/db_cleaner.js(1 hunks)src/server/object_services/md_store.js(5 hunks)src/util/postgres_client.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
PR: noobaa/noobaa-core#9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/server/object_services/md_store.js
🧬 Code graph analysis (1)
src/server/bg_services/db_cleaner.js (1)
src/server/object_services/md_store.js (3)
dbg(14-14)P(13-13)config(28-28)
🔇 Additional comments (10)
src/server/object_services/md_store.js (6)
779-779: LGTM! Read-only pool routing for unreclaimed objects.Correctly routes the find operation to the read-only pool, which aligns with the PR objective of offloading background read queries from the main cluster.
1127-1127: LGTM! Read-only pool routing for deleted objects query.The SQL query correctly uses the read-only pool via the options parameter, consistent with the pattern established in this PR.
1603-1603: LGTM! Read-only pool routing for chunk iteration.Appropriately routes the chunk iteration query to the read-only pool for the scrubber's background operations.
1778-1779: LGTM! Read-only pool routing for deleted chunks.Correctly adds
preferred_pool: 'read_only'to the find options for deleted chunks cleanup query.
1787-1788: LGTM! Read-only pool routing for chunk existence checks.Both
has_any_blocks_for_chunkandhas_any_parts_for_chunkcorrectly route to the read-only pool. These existence checks are read-only operations suitable for read replicas.Also applies to: 1796-1797
2039-2040: LGTM! Read-only pool routing for deleted blocks.Correctly routes the deleted blocks query to the read-only pool for background cleanup operations.
src/util/postgres_client.js (3)
252-252: Helpful debug logging for pool routing.The added debug log that includes
pg_client.options?.hostwill be useful for verifying which pool (and host) is being used for each query, aiding in troubleshooting read-only pool routing.
726-733: LGTM! Clean implementation of preferred_pool in executeSQL.The JSDoc and implementation correctly support routing queries to a preferred pool. The pattern of passing
options.preferred_pooltoget_pool()is clean and extensible.
937-937: LGTM! Consistent preferred_pool support in find and findOne.Both methods correctly pass
options.preferred_pooltoget_pool(), enabling read-only pool routing for these common query methods. The implementation is consistent with the executeSQL pattern.Also applies to: 954-954
src/sdk/nb.d.ts (1)
769-769: LGTM! TypeScript interface correctly updated.The addition of
preferred_pool?: stringto the executeSQL options parameter aligns with the implementation in postgres_client.js and properly documents the new functionality for TypeScript consumers.
Signed-off-by: Amit Prinz Setter <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/util/postgres_client.js (1)
635-638: Add warning log when falling back to default pool.The silent fallback could mask misconfigurations. If the
read_onlypool is expected but unavailable, queries will silently route to the default pool, defeating the load distribution purpose.Apply this diff to add visibility:
//if original get_pool was not for the default this.pool_key, try also this.pool_key if (key && key !== this.pool_key) { + dbg.warn(`Pool ${key} not available for table ${this.name}, falling back to default pool ${this.pool_key}`); return this.get_pool(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util/postgres_client.js(5 hunks)
⏰ 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). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (4)
src/util/postgres_client.js (4)
251-252: LGTM! Helpful debug logging for pool routing verification.The host logging will be useful for confirming that queries are being routed to the correct pool (read-only vs. default).
954-954: LGTM! Consistent with find implementation.The
preferred_poolsupport infindOneis consistent with thefindimplementation. Same eventual consistency considerations apply.
937-937: LGTM. Routingfindqueries to read replicas is appropriate for background maintenance operations.The callers using
preferred_pool: 'read_only'are background operations (find_unreclaimed_objects,find_deleted_objects, chunk/block existence checks) where eventual consistency is acceptable. These align with the stated use cases (object reclaimer, scrubber) and do not impact critical read paths.
726-733: Review comment is incorrect—executeSQL is designed for standalone queries and not used within transactions.The code shows that
executeSQLandPgTransactionhave separate execution paths:
PgTransaction(lines 335-378) uses its ownquery()method with a dedicatedpg_clientconnection for transactional queriesexecuteSQL(line 731) gets its own pool connection viapool.connect()for standalone queries- All
executeSQLcall sites in md_store.js are standalone queries, never invoked within transaction contexts- The
preferred_pooloption is appropriate for directing standalone queries to specific pools (e.g., read-only)There is no mixing of transaction and standalone query execution paths, and no risk of routing queries to incorrect pools during transactions.
Likely an incorrect or invalid review comment.
1 participant
Describe the Problem
Use new 'read only' pool (connected to cnpg replicated read-only cluster) in order to alleviate load from main, writable pg cluster.
Explain the Changes
-db cleancer
-object reclaimer's first find
-scrubber's first find
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
executeSQLmethod to accept an optional pool preference parameter, enabling flexible routing of queries to specific database pools for improved resource optimization.Improvements