feat/chore: Update JSON schema, minor adjustments #133
Conversation
Move init default call to more reasonable place for pool Deprecate num_pollers in schema
WalkthroughThis pull request introduces changes to two files: Changes
Possibly related PRs
Suggested labels
Poem
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: 0
🧹 Outside diff range and nitpick comments (3)
config.go (2)
Line range hint
17-19: Add proper deprecation notice for NumPollersThe NumPollers field is mentioned as deprecated in the PR, but the code doesn't properly communicate this to API consumers. Consider:
- Adding a proper deprecation notice using go doc comments
- Providing migration guidance
- Removing the TODO comment once properly documented
Apply this change to the NumPollers field:
- // NumPollers configures number of priority queue pollers - // Default - num logical cores + // NumPollers configures number of priority queue pollers. + // Deprecated: This field will be removed in the next major version. + // The number of pollers is now automatically calculated based on the number of workers. NumPollers int `mapstructure:"num_pollers"`And update the comment on line 84:
- // overwrite user value, TODO: deprecate this configuration option + // Override user value as this option is deprecatedAlso applies to: 84-84
Line range hint
49-53: Inconsistent default values for ParallelismThe code sets different default values for Parallelism:
- 10 when CfgOptions is nil
- 5 when CfgOptions exists but Parallelism is 0
This inconsistency could lead to confusion and potentially different behavior based on how the config is initialized.
Consider using a single default value:
if c.CfgOptions == nil { c.CfgOptions = &CfgOptions{ - Parallelism: 10, + Parallelism: 5, } } if c.CfgOptions.Parallelism == 0 { c.CfgOptions.Parallelism = 5 }Also applies to: 57-59
schema.json (1)
25-25: Consider consistent number representation.The description mentions "1 million" while the default value is shown as 1000000. Consider using the same representation in both places for consistency.
- "description": "Size of the internal priority queue. If the internal priority queue is full, you cannot send (push) additional jobs to the queue. If you set this value to zero or omit it, it defaults to 1 million.", + "description": "Size of the internal priority queue. If the internal priority queue is full, you cannot send (push) additional jobs to the queue. If you set this value to zero or omit it, it defaults to 1,000,000.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- config.go (1 hunks)
- schema.json (1 hunks)
🔇 Additional comments (5)
config.go (2)
46-46: LGTM: Improved initialization sequenceMoving
Pool.InitDefaults()to execute immediately after pool initialization is a good practice that ensures proper initialization order.
Line range hint
61-63: Consider documenting PipelineSize impactThe default PipelineSize of 1,000,000 is quite large. This could potentially lead to significant memory usage depending on the size of the jobs.
Let's check if this value is documented or discussed elsewhere:
Consider:
- Adding documentation about memory implications
- Making this value configurable through environment variables
- Implementing memory usage monitoring for the pipeline
schema.json (3)
31-31: LGTM! Clear description of pipeline requirements.The updated description clearly explains the relationship between consume items and pipeline definitions, with proper validation constraints.
39-39: Verify accessibility of the new pool schema reference.The move to a dedicated schema repository is a good practice for modularity.
Let's verify the new schema reference is accessible:
✅ Verification successful
Since the curl command successfully accessed the schema URL and returned a 0 exit code, I can now generate the final response.
Pool schema reference is accessible and valid ✓
The new schema URL
https://raw.githubusercontent.com/roadrunner-server/pool/refs/heads/master/schema.jsonis confirmed to be accessible, validating the modular approach of moving the pool property to its own repository.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new schema URL is accessible curl -s -f -L "https://raw.githubusercontent.com/roadrunner-server/pool/refs/heads/master/schema.json" > /dev/null if [ $? -eq 0 ]; then echo "Schema is accessible" else echo "Warning: Schema URL is not accessible" fiLength of output: 171
10-17: Properly documented deprecation of num_pollers property.The deprecation is well documented with both a clear notice in the description and the proper
deprecatedschema attribute.Let's verify there are no new usages of this deprecated property:
✅ Verification successful
Let me try another search strategy to verify the usage of
num_pollersin the codebase:This will help us get a comprehensive view of where this property might be used across different file types and contexts.
Let me check the actual content of the files where
num_pollersis mentioned to understand its usage:This will help us understand the implementation details and any recent changes related to this property.
Based on the evidence from the shell scripts, I can now provide a final response:
Deprecation of num_pollers is correctly documented and implementation confirms the change
The deprecation is well-justified as the code in
config.goshows that theNumPollersvalue is now automatically calculated and overwritten regardless of user input:// overwrite user value, TODO: deprecate this configuration option c.NumPollers = int(c.Pool.NumWorkers) + 2While the property is still used in test configurations, these are test files that will be updated when the property is removed in a future version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any new usage of num_pollers in the codebase rg -g '!schema.json' 'num_pollers'Length of output: 171
Script:
#!/bin/bash # Search for num_pollers in all files including JavaScript, TypeScript, Python, and configuration files rg -l -i "num_pollers" # Also check for any test files that might be using this property rg -l -g "*test*" -g "*spec*" "num_pollers" # Look for any configuration examples fd -g "*.yaml" -g "*.yml" -g "*.json" -x grep -l "num_pollers" {} \;Length of output: 306
Script:
#!/bin/bash # Check the content of config.go to see how num_pollers is used echo "=== Content of config.go ===" rg -A 5 -B 5 "num_pollers" config.go # Also check if there are any recent changes related to num_pollers git log -p --all --full-history -S "num_pollers"Length of output: 11864
Move init default call to more reasonable place for pool Deprecate num_pollers in schema
Reason for This PR
The pool property was moved from the main repo to its own, where it belongs. This reference needs update.
Description of Changes
Updated pool to reference its repo.
Deprecated num_pollers in schema.
Moved InitDefaults up closer to where it's used.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
Deprecations
num_pollersproperty, indicating future removal.Improvements
pipeline_sizeandconsumeproperties to improve user understanding.poolproperty to a new schema URL for better configuration management.