feat(subs): redis backend#855
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
==========================================
+ Coverage 77.08% 77.15% +0.07%
==========================================
Files 148 149 +1
Lines 17509 17717 +208
Branches 1721 1731 +10
==========================================
+ Hits 13497 13670 +173
- Misses 3993 4024 +31
- Partials 19 23 +4 ☔ View full report in Codecov by Sentry. |
michael-0acf4
force-pushed
the
redis-backend
branch
2 times, most recently
from
91d650d to
7a8cbd3
Compare
|
@coderabbitai review |
WalkthroughThe pull request introduces several changes primarily focused on integrating Redis as a new backend option within the project. This includes updates to configuration files, the addition of a Redis module, and modifications to existing structures and types to accommodate the new backend. The changes also extend to testing frameworks, ensuring comprehensive testing for the new Redis functionality alongside existing backends. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (24)
tools/compose/compose.subredis.yml (2)
1-8: Overall structure looks good, but some improvements are needed.The Docker Compose configuration for the Redis service is well-structured and uses appropriate settings for the most part. Here are some suggestions for improvement:
Password Security: Instead of hardcoding the Redis password, consider using environment variables or Docker secrets for better security.
Data Persistence: Add a volume to persist Redis data across container restarts.
Network Security: Consider using Docker networks to isolate the Redis service from external access if direct access from the host is not required.
Here's a suggested improvement to address these points:
services: subredis: image: bitnami/redis:7.0 restart: unless-stopped ports: - "6380:6379" environment: REDIS_PASSWORD: ${REDIS_PASSWORD} volumes: - redis_data:/bitnami/redis/data networks: - internal volumes: redis_data: networks: internal: internal: trueMake sure to set the
REDIS_PASSWORDenvironment variable when runningdocker-compose up, or use a.envfile.
3-3: Consider using a more specific image tag.While using the major version tag (7.0) is good, consider using a more specific tag (e.g., 7.0.11) for even better consistency and reproducibility. Remember to regularly update this tag as new versions are released.
- image: bitnami/redis:7.0 + image: bitnami/redis:7.0.11src/substantial/Cargo.toml (1)
15-15: LGTM! Consider updating to the latest minor version.The addition of the redis dependency aligns well with the PR objective of introducing a Redis backend. The dependency is correctly placed in the
[dependencies]section.Consider updating to the latest minor version (0.25.1 as of September 2024) for potential bug fixes and performance improvements:
-redis = "0.25.4" +redis = "0.25.1"tests/runtimes/substantial/kv_like_test.ts (2)
6-8: LGTM. Consider parameterizing the timeout value.The basic test for memory storage is set up correctly. The 10-second wait time (
awaitSleepCompleteSec: 10) should be sufficient for most scenarios.Consider parameterizing this value or explaining why 10 seconds was chosen. This could make the tests more flexible and easier to adjust in the future if needed.
10-12: LGTM. Consider if FS operations need a longer timeout.The basic test for file system storage is set up correctly and consistently with the memory storage test.
Consider if file system operations might require a longer timeout compared to memory operations. If not, it might be worth adding a comment explaining why the same timeout is suitable for both storage types.
tests/runtimes/substantial/redis_test.ts (1)
20-25: LGTM: Concurrent workflow test template is well-configured.The concurrent workflow test template for Redis is properly set up with appropriate parameters. The longer wait time of 20 seconds provides a good buffer for concurrent operations.
Consider adding more detailed comments about the test's expectations. For example:
concurrentWorkflowTestTemplate( "redis", - { awaitEmailCompleteSec: 20 }, // 2/3 will complete at <15s + { awaitEmailCompleteSec: 20 }, // Expect 2/3 of operations to complete in <15s, allowing 5s buffer for slower operations { SUB_REDIS }, redisCleanup(SUB_REDIS) );src/common/src/typegraph/runtimes/substantial.rs (2)
10-10: Approve the RedisConfig change with a suggestion for documentation.The modification to use a single
connection_stringinstead of separatehostandportfields is a good improvement. It simplifies configuration and allows for more flexible connection options.Consider adding a comment or updating the documentation to provide examples of valid connection string formats. This will help users understand how to properly configure the Redis backend.
Example:
/// Redis configuration /// /// # Examples /// /// ``` /// let config = RedisConfig { /// connection_string: "redis://127.0.0.1:6379".to_string(), /// }; /// ``` pub struct RedisConfig { pub connection_string: String, }
Line range hint
1-58: Summary of changes and recommendationsThe changes introduced in this file successfully support the addition of a Redis backend as per the PR objectives. The modifications to both the
RedisConfigstruct and theSubstantialBackendenum are well-considered and improve the overall design.To fully address the PR objectives and checklist items:
- Ensure that new or modified tests are added to cover the Redis backend functionality, particularly the serialization and deserialization of the
SubstantialBackendenum.- Add explanatory comments for the
RedisConfigstruct to clarify the expected format of theconnection_string.- Update the end-user documentation to reflect these changes, especially regarding how to configure the Redis backend using the new connection string format.
Consider the following architectural recommendations:
- Implement a method to validate the
connection_stringformat in theRedisConfigstruct to catch configuration errors early.- If not already present, create a trait or interface for backend operations to ensure consistency across different backend implementations (Fs, Memory, Redis).
These changes will enhance the robustness and maintainability of the Redis backend implementation.
tests/runtimes/substantial/substantial.py (1)
12-17: Approve backend initialization changes with a suggestion for error handling.The new backend initialization logic looks good and aligns with the PR objective of introducing a Redis backend. It provides more flexibility in backend selection using environment variables, which is a good practice.
Consider adding an
elseclause to handle unexpected values in theSUB_BACKENDenvironment variable. This would improve error handling and make debugging easier. Here's a suggested improvement:if "SUB_BACKEND" in os.environ: if os.environ["SUB_BACKEND"] == "fs": backend = Backend.dev_fs() elif os.environ["SUB_BACKEND"] == "redis": backend = Backend.redis("SUB_REDIS") + else: + raise ValueError(f"Unsupported backend type: {os.environ['SUB_BACKEND']}")src/substantial/src/backends/mod.rs (1)
Line range hint
1-78: Consider updating documentation for the new Redis backend.While the addition of the Redis module is correct, it's important to ensure that the project's documentation is updated to reflect this new backend option.
Consider adding a comment above the
pub mod redis;line to briefly explain its purpose, similar to how you might have comments for the other backend modules. Additionally, update any relevant documentation files (e.g., README.md) to mention the new Redis backend option.src/typegraph/deno/src/runtimes/substantial.ts (4)
16-18: Approve renaming todevMemoryand suggest documentation update.The renaming of
memorytodevMemoryis a good change as it clearly indicates that this is a development-only backend. This aligns with best practices for distinguishing between development and production code.Consider adding a brief comment explaining that this is for development use only, to enhance code clarity:
static devMemory(): SubstantialBackend { + // Returns a memory-based backend for development use only return { tag: "memory" }; }
20-22: Approve renaming todevFsand suggest documentation update.The renaming of
fstodevFsis consistent with the previous change and clearly indicates that this is a development-only backend. This maintains consistency in the codebase.Consider adding a brief comment explaining that this is for development use only, similar to the suggestion for
devMemory:static devFs(): SubstantialBackend { + // Returns a file system-based backend for development use only return { tag: "fs" }; }
24-31: Approve Redis backend implementation and suggest improvements.The implementation of the Redis backend looks good. The change simplifies the method signature and encapsulates the connection string secret, which is a good practice for handling sensitive information.
Consider the following improvements:
- Add input validation for the
connectionStringSecret:static redis(connectionStringSecret: string): SubstantialBackend { + if (!connectionStringSecret || typeof connectionStringSecret !== 'string') { + throw new Error('Invalid connection string secret'); + } return { tag: "redis", val: { connectionStringSecret, }, }; }
- Add a comment explaining the expected format of the connection string secret:
+ /** + * Creates a Redis backend using a connection string secret. + * @param connectionStringSecret - The secret containing the Redis connection string. + * Format: "redis://[[username][:password]@][host][:port][/database]" + * @returns A SubstantialBackend configuration for Redis + */ static redis(connectionStringSecret: string): SubstantialBackend { // ... implementation ... }
Line range hint
4-11: Suggest minor improvements to import organization.The import statements look correct and match the new implementations. However, consider the following suggestions for improved readability and maintainability:
- Separate external and internal imports with a blank line.
- Group related imports together.
- Consider using named imports consistently.
Here's a suggested reorganization:
// External imports (if any) // Internal imports import { type Materializer, Runtime } from "../runtimes/mod.ts"; import { runtimes } from "../wit.ts"; import { Func, type Typedef } from "../types.ts"; import { t } from "../index.ts"; import type { SubstantialBackend, SubstantialOperationData, SubstantialOperationType, Workflow, } from "../gen/typegraph_core.d.ts";src/typegraph/python/typegraph/runtimes/substantial.py (3)
29-30: Approve name change and suggest documentation update.The renaming of
memory()todev_memory()is a good change as it clarifies the method's intended use for development purposes. This aligns with the changes mentioned in the AI-generated summary.Consider adding a docstring to explain the purpose of this method and its development-specific nature. For example:
def dev_memory(): """ Returns a SubstantialBackendMemory instance for development use. This method should not be used in production environments. """ return SubstantialBackendMemory()
32-33: Approve name change and suggest documentation update.The renaming of
fs()todev_fs()is consistent with the previous change and clarifies the method's intended use for development purposes. This aligns with the changes mentioned in the AI-generated summary.Consider adding a docstring to explain the purpose of this method and its development-specific nature. For example:
def dev_fs(): """ Returns a SubstantialBackendFs instance for development use. This method should not be used in production environments. """ return SubstantialBackendFs()
29-37: Summary of changes and potential impactThe changes in this file improve the clarity of the
Backendclass methods:
memory()andfs()have been renamed todev_memory()anddev_fs()respectively, clearly indicating their development-specific nature.- The
redis()method signature has been changed to accept a connection string secret instead of aRedisBackendinstance, simplifying its usage.These changes align with the PR objectives of introducing a Redis backend for the project. However, there are a few points to consider:
- The renaming of
memory()andfs()methods might affect existing code that uses these methods. Ensure that all references to these methods are updated accordingly.- The change in the
redis()method signature might require updates in other parts of the codebase whereRedisBackendinstances were being passed.- As per the PR checklist, ensure that:
- New or modified tests are included to cover these changes.
- Hard-to-understand functions (if any) have explanatory comments.
- End-user documentation is updated to reflect these changes, especially the new Redis backend implementation.
Consider adding a deprecation warning for the old method names (
memory()andfs()) to ease the transition for existing code. This can be done using thewarningsmodule:import warnings class Backend: @staticmethod def memory(): warnings.warn("The 'memory()' method is deprecated. Use 'dev_memory()' instead.", DeprecationWarning, stacklevel=2) return Backend.dev_memory() @staticmethod def fs(): warnings.warn("The 'fs()' method is deprecated. Use 'dev_fs()' instead.", DeprecationWarning, stacklevel=2) return Backend.dev_fs() # ... rest of the class ...This approach will allow for a smoother transition and give users of your library time to update their code.
.github/workflows/tests.yml (1)
Line range hint
1-292: Summary: Redis backend integration looks good. Consider broader impact.The change to include
subredisin the dev-compose command is the only modification in this workflow file and aligns well with the PR objective of introducing a Redis backend. However, to ensure a comprehensive implementation:
- Verify that all necessary Redis-related code changes have been made in other parts of the codebase.
- Ensure that appropriate tests have been added or updated to cover the new Redis functionality.
- Check if any documentation updates are required to reflect the addition of the Redis backend.
To help with this verification, you can run the following command to get an overview of Redis-related changes across the codebase:
#!/bin/bash # Description: Check for Redis-related changes across the codebase # Search for files containing 'redis' or 'subredis' (case-insensitive) rg -i '(redis|subredis)' --type-add 'code:*.{py,rs,ts,js}' -t codeThis will help identify areas that might need additional review or testing to ensure a robust implementation of the Redis backend.
tests/utils/test.ts (1)
92-92: LGTM! Consider adding JSDoc for the exported type.The change to export the
MetaTestCleanupFntype is appropriate and potentially useful for other modules. It allows for better type checking and consistency when working with cleanup functions across the project.Consider adding JSDoc comments to describe the purpose and usage of this type, especially now that it's exported. For example:
/** * Represents a cleanup function for MetaTest. * @returns A promise or unknown value after cleanup is complete. */ export type MetaTestCleanupFn = () => unknown | Promise<unknown>;src/substantial/src/backends/redis.rs (5)
16-20: Remove Underscore Prefix from Used Struct Field_conThe field
_coninRedisBackendis used throughout the implementation. Prefixing it with an underscore suggests it's unused, which can be misleading. Consider renaming_contoconfor clarity.Apply this diff to rename the field:
- _con: Mutex<redis::Connection>, + con: Mutex<redis::Connection>,Also, update all usages of
_contocon.
228-230: Address the FIXME: Refactor Lua Script innext_runMethodA FIXME comment indicates the need to refactor the Lua script due to limitations with using
ARGVin certain contexts:// FIXME: refactor this out, one blocker being that // redis will complain if ARGV is used in any 'indirect' form or manner // such as within a function (even local): "Attempt to modify a readonly table script"Consider refactoring the Lua script to improve maintainability and resolve the issue with
ARGVusage.Would you like assistance in rewriting the Lua script or exploring alternative approaches? I can help address this and open a new GitHub issue to track the task if needed.
27-30: Implement Connection Pooling for Improved PerformanceThere's a TODO comment noting the need for a connection pool to enhance performance:
// TODO: use a connection pool, this can get noticeably slow even locally // There is r2d2-redis but it's very outdated as of nowUsing a connection pool will help manage multiple concurrent connections efficiently and improve the backend's responsiveness.
I can suggest up-to-date connection pooling libraries compatible with Redis and help implement this feature. Would you like me to open a GitHub issue to track this enhancement?
473-473: Remove Unnecessary Command in Lua ScriptAfter correcting the previous issue, the
ZREMcommand is no longer needed and should be removed to prevent unintended side effects.Apply this diff to remove the unnecessary command:
-redis.call("ZREM", sched_key, content)
163-166: Unused Parameter_queueinread_scheduleThe parameter
_queueis unused in theread_schedulemethod:fn read_schedule(&self, _queue: String, run_id: String, schedule: DateTime<Utc>) -> Result<Option<Event>> {
- Prefixing with an underscore suppresses the compiler warning, but it might indicate a design issue if the queue context is necessary.
Confirm whether the
queueparameter is needed. If not, consider removing it from the method signature to simplify the API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
- .github/workflows/tests.yml (1 hunks)
- src/common/src/typegraph/runtimes/substantial.rs (1 hunks)
- src/substantial/Cargo.toml (1 hunks)
- src/substantial/src/backends/mod.rs (1 hunks)
- src/substantial/src/backends/redis.rs (1 hunks)
- src/substantial/src/operations.rs (0 hunks)
- src/substantial/tests/mod.rs (6 hunks)
- src/typegate/engine/runtime.d.ts (1 hunks)
- src/typegate/engine/src/runtimes/substantial.rs (16 hunks)
- src/typegate/src/runtimes/substantial.ts (5 hunks)
- src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (1 hunks)
- src/typegraph/core/src/conversion/runtimes.rs (1 hunks)
- src/typegraph/core/wit/typegraph.wit (1 hunks)
- src/typegraph/deno/src/runtimes/substantial.ts (2 hunks)
- src/typegraph/python/typegraph/runtimes/substantial.py (1 hunks)
- tests/runtimes/substantial/common.ts (1 hunks)
- tests/runtimes/substantial/kv_like_test.ts (1 hunks)
- tests/runtimes/substantial/redis_test.ts (1 hunks)
- tests/runtimes/substantial/substantial.py (1 hunks)
- tests/runtimes/substantial/substantial_test.ts (0 hunks)
- tests/runtimes/substantial/workflow.ts (1 hunks)
- tests/utils/test.ts (1 hunks)
- tools/compose/compose.subredis.yml (1 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- src/substantial/src/operations.rs
- tests/runtimes/substantial/substantial_test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/runtimes/substantial/workflow.ts
🔇 Additional comments (23)
tools/compose/compose.subredis.yml (1)
6-6: Verify the necessity of exposing Redis port.Exposing the Redis port to the host might pose security risks if not properly protected. If Redis is only used internally by other services in the same Docker network, consider removing the port mapping.
To check if other services in the project require direct access to Redis from the host, run:
If no results are found, you may safely remove the
portssection.tests/runtimes/substantial/kv_like_test.ts (1)
14-16: LGTM. Clarify parameter naming and test coverage.The concurrent workflow test for file system storage is set up correctly.
The timeout parameter is named
awaitEmailCompleteSechere, while it'sawaitSleepCompleteSecin the basic tests. Is this intentional? If so, consider adding a comment explaining the difference.There's no concurrent workflow test for memory storage. Is this intentional? If not, consider adding one for consistency.
Let's verify if there are any other concurrent workflow tests:
tests/runtimes/substantial/redis_test.ts (3)
1-9: LGTM: File structure and imports are well-organized.The file structure is clean, with proper copyright and license information. The use of a common file for shared test utilities promotes code reuse and maintainability.
1-25: Overall, the Redis test file is well-structured and comprehensive.This new test file for Redis functionality is well-organized and covers both basic and concurrent workflow scenarios. The use of shared utilities from a common file promotes code reuse, and the inclusion of cleanup functions ensures proper test isolation. The test configurations appear appropriate for their respective scenarios.
A few minor suggestions have been made to enhance the file:
- Verify if the 10-second sleep duration in the basic test is sufficient for all Redis operations.
- Consider adding more detailed comments about the expectations in the concurrent workflow test.
These enhancements will further improve the clarity and robustness of the tests.
11-18: LGTM: Basic test template is well-configured.The basic test template for Redis is properly set up with appropriate parameters. The use of a cleanup function ensures proper test isolation.
Consider verifying if the 10-second sleep duration is sufficient for all potential Redis operations in this basic test scenario. You may want to run the following command to check for any timeout-related issues in the test results:
✅ Verification successful
Verified: The 10-second sleep duration in the Redis test is sufficient.
No timeout-related issues were found in the Redis test suite, indicating that the current sleep duration adequately accommodates all necessary operations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for timeout-related issues in test results # Test: Search for timeout or duration-related messages in test output rg --type ts "timeout|duration|exceeded" tests/Length of output: 2391
tests/runtimes/substantial/substantial.py (2)
Line range hint
1-45: Overall assessment: Changes look good with minor suggestions.The implementation of the Redis backend and the updates to the dependency paths are well done. The changes align with the PR objectives and improve the flexibility of the backend selection. The code is clear and easy to understand.
To further improve the code:
- Consider adding error handling for unexpected
SUB_BACKENDvalues.- Verify the consistency of the new import path (
imports/common_types.ts) across the codebase.Great job on implementing these features!
24-24: Approve dependency updates with a suggestion for verification.The updates to the
depsparameter in the Deno method calls look good. The change fromcommon_types.tstoimports/common_types.tsis consistent across both calls.To ensure that this change doesn't introduce any issues, please verify that the new import path is correct and that it's been updated consistently across the codebase. Run the following script to check for any remaining references to the old path:
If the script returns any results for the first command or fails to find the file in the second command, please review and update the affected files accordingly.
Also applies to: 30-30
src/substantial/src/backends/mod.rs (2)
Line range hint
1-78: Verify trait implementations for the Redis backend.To ensure the Redis backend is fully functional, it should implement all the necessary traits defined in this file:
BackendStore,BackendMetadataWriter,BackendAgent, andBackend.Let's verify the trait implementations for the Redis backend:
#!/bin/bash # Description: Verify trait implementations for the Redis backend # Check for BackendStore implementation ast-grep --lang rust --pattern 'impl BackendStore for Redis { $$$$ }' # Check for BackendMetadataWriter implementation ast-grep --lang rust --pattern 'impl BackendMetadataWriter for Redis { $$$$ }' # Check for BackendAgent implementation ast-grep --lang rust --pattern 'impl BackendAgent for Redis { $$$$ }' # Check for Backend implementation ast-grep --lang rust --pattern 'impl Backend for Redis { $$$$ }'If any of these implementations are missing, please ensure they are added to fully integrate the Redis backend.
14-14: New Redis backend module added.The addition of the
redismodule expands the backend options for the project, which is in line with the PR objective of introducing a Redis backend.To ensure proper integration of the Redis backend, let's verify its implementation:
src/typegraph/deno/src/runtimes/substantial.ts (1)
Line range hint
1-131: Summary of changes and their impact.The changes in this file successfully introduce a Redis backend and improve the clarity of the development-only backends. Here's a summary of the key modifications:
- Renamed
memorytodevMemoryandfstodevFs, clearly indicating their development-only nature.- Updated the
redismethod to accept a connection string secret, improving the handling of sensitive information.- Adjusted import statements to match the new implementations.
These changes align well with the PR objectives and follow good practices for code clarity and security. The rest of the file, including the
SubstantialRuntimeandWorkflowHandleclasses, remains unchanged and appears to be compatible with the new backend implementations.To further improve the code:
- Consider adding brief comments to the
devMemoryanddevFsmethods.- Implement input validation and add documentation for the
redismethod.- Reorganize the import statements for better readability.
Overall, these changes represent a solid improvement to the backend implementation.
🧰 Tools
🪛 Biome
[error] 15-32: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/typegraph/python/typegraph/runtimes/substantial.py (1)
35-37: Approve signature change and suggest documentation and verification.The change in the
redis()method signature fromredis(config: RedisBackend)toredis(connection_string_secret: str)simplifies the method call and aligns with the changes mentioned in the AI-generated summary. This new implementation provides better encapsulation by creating theRedisBackendinstance internally.Consider adding a docstring to explain the purpose of this method and the expected format of the
connection_string_secret. For example:def redis(connection_string_secret: str): """ Returns a SubstantialBackendRedis instance configured with the provided connection string secret. Args: connection_string_secret (str): The secret containing the Redis connection string. Returns: SubstantialBackendRedis: A configured Redis backend instance. """ return SubstantialBackendRedis(value=RedisBackend(connection_string_secret))This change in method signature might affect existing code that was passing a
RedisBackendinstance. Please verify the impact on the codebase:If any results are found, they will need to be updated to pass a connection string secret instead of a
RedisBackendinstance.src/typegate/src/runtimes/substantial/workflow_worker_manager.ts (2)
Line range hint
1-247: Summary of reviewThe changes made to the
workflow_worker_manager.tsfile are minimal and focused on adjusting the logging behavior in thedestroyWorkermethod of theWorkflowRecorderclass. While the change itself is small, it's important to consider its implications on error tracking and system monitoring.Key points:
- The logging level change from error to warning has been addressed.
- Suggestions for improving clarity and maintainability have been provided.
- Recommendations for testing and verifying the change have been outlined.
Overall, the change appears to be a reasonable adjustment to the logging strategy, but care should be taken to ensure it aligns with the project's overall error handling and monitoring approach.
The changes can be approved once the suggested improvements and testing recommendations have been considered and implemented as appropriate.
Line range hint
1-247: Overall impact and testing recommendationsThe change in the
destroyWorkermethod of theWorkflowRecorderclass is relatively minor but could have implications for error tracking and system monitoring. To ensure the change doesn't introduce any regressions or unintended consequences:
- Update existing unit tests for the
destroyWorkermethod to verify the new logging behavior.- Add integration tests to confirm that the change doesn't affect the overall workflow destruction process.
- Review and update any log parsing or alerting systems that may have been relying on the previous error-level log message.
- Consider adding a feature flag to toggle between the old and new logging behavior, allowing for easier rollback if issues are discovered in production.
To ensure that this is the only instance where this logging change was made, run the following command:
This will help identify any other locations where similar logging changes might be beneficial for consistency.
✅ Verification successful
Verification Complete: No Additional Instances Found
The changes in the
destroyWorkermethod of theWorkflowRecorderclass have been successfully verified. No other instances oflogger.errorwere found that require similar modifications related to this review comment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of logger.error that might need similar changes rg --type typescript 'logger\.error' -C 3Length of output: 78
Script:
#!/bin/bash # Search for other instances of logger.error in TypeScript files (.ts and .tsx) rg 'logger\.error' --glob '*.ts' --glob '*.tsx' -C 3Length of output: 17372
src/typegate/engine/runtime.d.ts (1)
295-299: Approve changes with recommendations for implementation and documentation.The restructuring of the
Backendtype improves consistency and flexibility. However, these are breaking changes that require careful implementation and documentation updates.
- The changes are approved for their improved consistency and flexibility.
- This is a breaking change that will require updates wherever the
Backendtype is used.- Add a comment to explain the expected format of the Redis connection string, e.g.:
/** * Redis connection string format: redis[s]://[[username][:password]@][host][:port][/db-number] */ connection_string: string;- Update all usages of the
Backendtype throughout the codebase.- Modify the Redis connection logic to parse and use the new connection string format.
- Update tests and documentation to reflect these changes, including any migration guides for users of the library.
To verify the impact of these changes, you can run the following script:
This script will help identify files that need to be updated due to the
Backendtype changes and Redis connection string modification.src/typegraph/core/src/conversion/runtimes.rs (1)
509-509: Improved Redis configuration with connection stringThe change to use a single
connection_string_secretfor Redis configuration is a good improvement. It simplifies the setup and allows for more flexible Redis configurations.However, consider the following points:
- Ensure that any existing code or documentation referencing separate
hostandportfields is updated accordingly.- Provide clear migration instructions for users who may be using the old configuration format.
- Verify that error handling and logging mechanisms do not accidentally expose the connection string, as it may contain sensitive information.
To ensure a smooth transition, please run the following verification script:
src/typegraph/core/wit/typegraph.wit (1)
491-493: Approve the change toredis-backendand suggest follow-up actionsThe modification to use a
connection-string-secretinstead of separatehostandportfields is a good improvement. It enhances security by treating the entire connection information as a secret and provides more flexibility in specifying connection details.However, to ensure a smooth transition, consider the following actions:
- Update any code that interacts with the
redis-backendto use the new connection string format.- Provide migration guidelines for users who need to update their configurations.
- Consider adding a comment or documentation explaining the expected format of the connection string (e.g., "redis://[[username][:password]@][host][:port][/database]").
To further improve the system's flexibility and security:
- Consider implementing a connection string parser to validate the format and extract individual components if needed elsewhere in the code.
- Ensure that the secret management system used can handle rotation of these connection string secrets.
To ensure consistency across the codebase, let's check for any other Redis-related configurations or usages:
✅ Verification successful
Verified removal of
hostandportinredis-backendThe
redis-backendrecord no longer includes thehostandportfields and has been successfully replaced withconnection-string-secret. This change enhances security by consolidating connection details into a single secret field.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Redis-related code rg --type=wit 'redis|Redis' -C 5Length of output: 62
Script:
#!/bin/bash # Search for Redis-related code across all files with context rg -i 'redis|Redis' -C 5Length of output: 168296
Script:
#!/bin/bash # Verify that 'redis-backend.host' and 'redis-backend.port' have been removed or replaced rg -i 'redis-backend\.host|redis-backend\.port' -C 5Length of output: 54
src/substantial/tests/mod.rs (2)
18-19: Function renamed for clarityRenaming the test function to
test_basic_write_and_read_events_raw_fsimproves clarity by indicating that this test covers basic functionality.
36-37: Function renamed for clarityRenaming the test function to
test_basic_run_persistimproves clarity by indicating that this test covers basic functionality.src/typegate/src/runtimes/substantial.ts (4)
Line range hint
19-25: Renaming interface enhances clarityRenaming
QueryWorkflowResulttoQueryCompletedWorkflowResultimproves code readability by explicitly indicating that this interface represents the result of a completed workflow.
29-33: Introduction ofQueryOngoingWorkflowResultimproves type safetyDefining
QueryOngoingWorkflowResultby omittingresultandended_atfromQueryCompletedWorkflowResultprovides a clear distinction between ongoing and completed workflows, enhancing type safety and maintainability.
214-215: Explicit typing ofongoingandcompletedarrays enhances type safetyAssigning explicit types to
ongoingandcompletedarrays usingQueryOngoingWorkflowResultandQueryCompletedWorkflowResultimproves type safety and code clarity.
77-81:⚠️ Potential issueEnsure correct usage of
backend.connection_stringwith secretsWhen the backend type is
"redis", the connection string is retrieved usingsecretManager.secretOrFail(backend.connection_string). Please verify thatbackend.connection_stringcontains the name of the secret key rather than the actual connection string value, to prevent misconfiguration.Run the following script to check where
backend.connection_stringis set and ensure it contains the secret key name:✅ Verification successful
Verification Passed:
backend.connection_stringcorrectly uses secret keysAll assignments to
backend.connection_stringutilizesecretManager.secretOrFail, and no direct string assignments were found, ensuring secure usage of secrets.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `backend.connection_string` is assigned the secret key name, not the actual connection string. # Search for assignments to `backend.connection_string` rg --type ts 'backend\.connection_string\s*=\s*' # Search for backend configuration where `connection_string` is set rg --type ts 'backend\s*=\s*.*\{.*connection_string\s*:'Length of output: 212
Script:
#!/bin/bash # Description: # 1. Verify that all assignments to `backend.connection_string` use `secretManager.secretOrFail`. # 2. Ensure that `secretManager.secretOrFail` is called with secret key names, not literal connection strings. # Find all assignments to `backend.connection_string` rg --type ts 'backend\.connection_string\s*=' # Find all usages of secretManager.secretOrFail and display the arguments passed rg --type ts 'secretManager\.secretOrFail\s*\(' -A 2Length of output: 5238
Script:
#!/bin/bash # Description: # 1. Find all assignments to `backend.connection_string` that use `secretManager.secretOrFail`. # 2. Identify any direct assignments of string literals to `backend.connection_string` without using `secretManager.secretOrFail`. # 1. Assignments using secretManager.secretOrFail echo "Assignments using secretManager.secretOrFail:" rg --type ts 'backend\.connection_string\s*=\s*secretManager\.secretOrFail\(' echo "" # 2. Direct assignments of string literals echo "Direct assignments of string literals to backend.connection_string:" rg --type ts 'backend\.connection_string\s*=\s*["'\''`][^"'\''`]+["'\''`]'Length of output: 516
src/substantial/src/backends/redis.rs (1)
496-499: Ensure Consistent Use of Sorted SetsIn
write_workflow_link,zaddis used but without specifying a score:r.zadd(links_key, run_id, 0)?;
- Ensure that the score value aligns with how retrieval operations expect the data.
- If ordering isn't required, consider using a Redis Set instead.
Confirm whether using a sorted set is necessary here. If ordering isn't needed, you might replace
zaddwithsaddfor a Redis Set.
2 participants
SUBSTANTIAL_POLL_INTERVAL_SECandSUBSTANTIAL_LEASE_LIFESPAN_SECto configMigration notes
Backend.fs()andBackend.memory()toBackend.dev_fs()andBackend.dev_memory()SUBSTANTIAL_RELAUNCH_MSas it was relevant only for purely worker-based runs, which rendered the newSUBSTANTIAL_POLL_INTERVAL_SECredundant when an interrupt hits.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores