Add resource limits and configuration validation#46
Conversation
- Add ResourceLimits struct to control concurrent batches, memory usage, and processing time - Add Validate() method to ConfigValues to ensure valid configuration - Add NewWithOptions() constructor to create Batch with resource limits - Add comprehensive tests for validation and resource tracking - Update Batch to enforce resource limits during processing This helps prevent resource exhaustion and ensures stable operation under heavy load.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @MasterOfBinary, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly. This PR, authored by MasterOfBinary, introduces significant enhancements to the batch processing library by adding resource limits and configuration validation. The goal is to provide better control over resource consumption (like concurrent batches, memory, and processing time) and to ensure that the batch configuration itself is valid before processing begins. It also includes a new constructor NewWithOptions to make it easier to configure a Batch with both standard configuration and the new resource limits.
Highlights
- Resource Limits: Introduces a
ResourceLimitsstruct allowing users to set constraints onMaxConcurrentBatches,MaxMemoryPerBatch,MaxProcessingTime, andMaxTotalMemory. Includes aDefaultResourceLimits()function to provide sensible defaults based on system resources. - Configuration Validation: Adds a
Validate()method toConfigValuesto check for common configuration errors, such asMinItemsbeing greater thanMaxItemsor negative time durations. Validation is performed whenBatch.Go()is called, failing fast if the configuration is invalid. - New Constructor: Adds
NewWithOptions()and anOptionsstruct to provide a clear way to instantiate aBatchwith bothConfigandResourceLimits. This maintains backward compatibility with the existingNew()constructor. - Resource Tracking: Implements an internal
resourceTrackerto monitor active batches and estimated memory usage, enforcing the configuredResourceLimitsduring batch processing. Batches exceeding limits are dropped with an error.
Changelog
Click here to see the changelog
- batch/batch.go
- Added
fmtimport. - Added
resourceTrackerfield to theBatchstruct. - Added configuration validation check at the beginning of the
Gomethod. - Integrated resource limit checks (
canStartBatch) before starting a batch goroutine. - Added
startBatchandfinishBatchcalls within the batch processing goroutine. - Added context timeout for
Processor.Processbased onMaxProcessingTimelimit. - Passed
estimatedSizeto the batch processing goroutine.
- Added
- batch/config.go
- Added
errorsandfmtimports. - Added
Validate()method toConfigValuesto check for invalid item counts and time durations.
- Added
- batch/config_validation_test.go
- Added new file for testing configuration validation.
- Includes unit tests for
ConfigValues.Validate()covering various valid and invalid scenarios. - Includes a test case for
Batch.Godemonstrating that it fails fast with an error for invalid configuration.
- batch/options.go
- Added new file defining the
Optionsstruct. - Added
WithDefaults()method toOptionsfor setting default configuration. - Added
NewWithOptions()constructor for creating aBatchwithOptions.
- Added new file defining the
- batch/resource_limits.go
- Added new file defining
ResourceLimitsstruct. - Added
DefaultResourceLimits()function based on system resources. - Added
Validate()method toResourceLimits. - Added
resourceTrackerstruct and its methods (newResourceTracker,canStartBatch,startBatch,finishBatch,getUsage) for tracking and enforcing limits.
- Added new file defining
- batch/resource_limits_test.go
- Added new file for testing resource limits and tracking.
- Includes unit tests for
ResourceLimits.Validate(). - Includes tests for the
resourceTracker's behavior under concurrent batch and memory limits. - Includes integration tests for
Batchrespecting concurrent batch and processing time limits.
- batch/test_helpers_internal_test.go
- Added new internal test helper file.
- Includes
ProcessorFuncandSourceFunctypes to easily create mockProcessorandSourceimplementations for testing.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Limits set with care,
Validation, a safety net,
Batches run just right.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This pull request adds configurable resource limits and configuration validation to the batch processing library, and introduces a new constructor to wire these options together.
- Enforce limits on concurrent batches, memory usage, and processing time
- Validate batch configuration parameters and fail fast on invalid input
- Introduce
NewWithOptions()andOptionsto combine config and resource limits
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| batch/test_helpers_internal_test.go | Add ProcessorFunc and SourceFunc test helpers to implement interfaces |
| batch/resource_limits_test.go | Add tests for ResourceLimits.Validate and resource tracker behavior |
| batch/resource_limits.go | Implement ResourceLimits, default values, validation, and tracker |
| batch/options.go | Define Options and NewWithOptions() to apply resource limits |
| batch/config_validation_test.go | Tests for ConfigValues.Validate and invalid-config behavior in Batch.Go |
| batch/config.go | Add Validate() to ConfigValues |
| batch/batch.go | Integrate config validation and resource limits into batch execution |
Comments suppressed due to low confidence (1)
batch/resource_limits_test.go:232
- The test runs the pipeline but doesn't assert on
errorCount; consider adding a check to verify the expected number of errors or that no resource limit errors occurred.
// With limit of 1 concurrent batch, processing should succeed
| }) | ||
| } | ||
|
|
||
| // Use the ProcessorFunc and SourceFunc types defined in config_validation_test.go No newline at end of file |
There was a problem hiding this comment.
This comment references the wrong file; the helper types are defined in test_helpers_internal_test.go, not config_validation_test.go.
| // Use the ProcessorFunc and SourceFunc types defined in config_validation_test.go | |
| // Use the ProcessorFunc and SourceFunc types defined in test_helpers_internal_test.go |
| close(b.done) | ||
| b.running = false |
There was a problem hiding this comment.
Assignment to the shared running field should be protected by the mutex to avoid race conditions (e.g., wrap in b.mu.Lock()/Unlock()).
| close(b.done) | |
| b.running = false | |
| close(b.done) | |
| b.mu.Lock() | |
| b.running = false | |
| b.mu.Unlock() |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable features for resource limiting and configuration validation, significantly enhancing the robustness of the batch processing system. The code is generally well-structured, and the addition of NewWithOptions improves the API for configuring these new features. The tests cover many scenarios, which is commendable.
I've identified a few areas for improvement, primarily concerning a discrepancy in configuration handling and the precision of resource estimation, along with a suggestion to strengthen one of the new tests.
Overall, great work on adding these important controls!
Summary of Findings
- Configuration Handling Discrepancy (
MinItems = 0): There's a conflict between the documented behavior ofMinItems = 0(process immediately) inConfigValues.Validateand its actual handling byfixConfig, which changes it to1. This could lead to unexpected batching behavior. - Memory Estimation Accuracy: The fixed 1KB per item memory estimation is a rough heuristic and might lead to inaccurate resource limiting if item sizes vary significantly.
- Test Assertion for Concurrent Batch Limit: The test
respects concurrent batch limitinresource_limits_test.godoes not fully assert the expected error conditions when the limit is hit, potentially missing verification of the limit enforcement. - Misleading Comment in Test File: A comment in
resource_limits_test.go(line 292) incorrectly states thatProcessorFuncandSourceFuncare defined inconfig_validation_test.go. They are actually defined in the newtest_helpers_internal_test.gofile. This was not commented on directly due to review settings.
Merge Readiness
This pull request makes significant improvements by adding resource limits and configuration validation. However, there is a high-severity issue regarding the handling of MinItems = 0 that needs to be addressed to ensure consistent and clear behavior. Additionally, a medium-severity concern about memory estimation and a test that could be strengthened should be considered.
I recommend addressing these points, especially the MinItems = 0 discrepancy, before merging. As a reviewer, I am not authorized to approve pull requests; please ensure further review and approval from authorized maintainers after addressing the feedback.
| // Validate checks if the configuration values are valid. | ||
| // It returns an error if any validation rules are violated. | ||
| // Note: MinItems of 0 is valid and means process items immediately. | ||
| func (c ConfigValues) Validate() error { |
There was a problem hiding this comment.
There seems to be a discrepancy regarding the behavior of MinItems = 0.1
The comment here (line 74) states: // Note: MinItems of 0 is valid and means process items immediately.
However, the fixConfig function in batch.go (line 439-441) changes MinItems = 0 to MinItems = 1 before the batching logic in waitForItems is called.
This means the documented behavior of "process items immediately" for MinItems = 0 might not be what actually occurs. If MinItems = 0 is indeed intended to have a special meaning (like processing items one by one as they arrive, potentially bypassing some timer logic), then fixConfig should not alter it, and waitForItems would need to handle it accordingly.
If, on the other hand, an effective MinItems of 1 is always desired internally for the batching loop, then this comment should be updated to reflect that, or Validate should perhaps enforce MinItems >= 1 (or clarify that 0 will be treated as 1).
Could you clarify the intended behavior for MinItems = 0 and ensure the code and documentation are consistent?
Style Guide References
Footnotes
| var estimatedSize int64 | ||
| if b.resourceTracker != nil { | ||
| // Estimate batch size (rough estimate: 1KB per item) | ||
| estimatedSize = int64(len(batch)) * 1024 |
There was a problem hiding this comment.
The memory estimation for a batch is currently int64(len(batch)) * 1024, assuming 1KB per item. This is acknowledged as a "rough estimate" in the comment.
While simple, this fixed estimation might not be accurate for all use cases. If items are much larger or smaller, this could lead to:
- Underestimation: Potentially exceeding actual memory limits.
- Overestimation: Unnecessarily restricting batch processing or dropping batches.
Given that MaxMemoryPerBatch and MaxTotalMemory are described as "soft limits," the impact might be somewhat mitigated. However, have you considered if there are scenarios where this roughness could be problematic, or if there's a path to allow users to provide a more accurate size estimation for their specific item types in the future (e.g., via an optional interface on items or a configurable estimation function)?
For the current implementation, perhaps the comment could briefly mention the potential implications of this estimation's accuracy?
| errs := b.Go(context.Background(), src, proc) | ||
|
|
||
| var errorCount int | ||
| for err := range errs { | ||
| if err != nil { | ||
| errorCount++ | ||
| } | ||
| } | ||
|
|
||
| // With limit of 1 concurrent batch, processing should succeed | ||
| // but may have resource limit errors if batches tried to run concurrently | ||
| <-b.Done() |
There was a problem hiding this comment.
In the test respects concurrent batch limit, errorCount is incremented when errors are received, but there are no assertions on its final value.
Given MaxConcurrentBatches: 1, and the source producing 4 items batched by 2 (so, 2 batches), with a slow processor:
- The first batch should start.
- When the second batch is ready,
canStartBatchshould fail because the first batch is still active and the limit is 1. - This should result in errors being sent to the
errschannel: one general "resource limit exceeded" error, and errors for each item in the dropped batch.
It would be good to assert that the expected number and type of errors are received. For example, you might expect at least one error indicating the concurrent batch limit was hit, and potentially errors for the items in the second batch being dropped.
Could this test be updated to assert the expected error behavior when the concurrent batch limit is enforced?
- Remove trailing whitespace - Fix indentation in struct fields - Ensure consistent formatting throughout
|
Closing as stale/conflicting with current master. If resource limits/config validation are still desired, please reopen in a fresh PR with current API compatibility. |
Summary
Changes
Resource Limits
ResourceLimitsstruct with the following controls:MaxConcurrentBatches: Limit number of batches processed concurrentlyMaxMemoryPerBatch: Soft limit on memory usage per batchMaxProcessingTime: Maximum time allowed for processing a single batchMaxTotalMemory: Soft limit on total memory usage across all batchesDefaultResourceLimits()function that provides sensible defaults based on system resourcesConfiguration Validation
Validate()method toConfigValuesto check for invalid configurations:Go()is called, failing fast with clear error messagesNew Constructor
NewWithOptions()to create a Batch with both configuration and resource limitsOptionsstruct to encapsulate both Config and ResourceLimitsTest plan
🤖 Generated with Claude Code