Extract RetryConfig and RetryPolicy to executor/retry.rs module #13

2025-07-05T13:53:22-07:00

jwilger commented

2025-07-05 13:53:22 -07:00

(Migrated from github.com)

Summary

Extract RetryConfig and RetryPolicy types to dedicated executor/retry.rs module
First small step in systematic executor.rs refactoring (2,956 lines → smaller modules)
Maintains all existing APIs and functionality - no breaking changes

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Refactoring (non-breaking change that improves code organization)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Performance improvement
Test improvement

Testing Checklist

Unit tests pass locally
Integration tests pass locally
Property-based tests pass locally
All examples compile and run successfully
Benchmarks show no performance regressions
Memory usage remains stable under load

Performance Impact

No performance impact expected - pure code organization change with identical runtime behavior.

Security Checklist

Input validation requirements reviewed
Authentication/authorization impacts assessed
Data encryption requirements verified
Sensitive data exposure risks evaluated
Audit logging requirements confirmed

Code Quality

Code follows established patterns and conventions
Complex logic is well-documented
Error handling is comprehensive
Resource cleanup is handled properly
Thread safety considerations addressed

Reviewer Checklist

Business logic changes reviewed for correctness
Performance implications assessed
Security implications reviewed
Test coverage adequate for changes
Documentation updated as needed
Breaking changes properly communicated
Migration guide provided if needed

Review Focus

This is a pure refactoring with no functional changes. Focus on:

Verify module organization makes sense
Confirm no behavioral changes
Check that imports/exports are correct
Validate this follows the systematic refactoring plan from PLANNING.md

🤖 Generated with Claude Code

## Summary - Extract RetryConfig and RetryPolicy types to dedicated executor/retry.rs module - First small step in systematic executor.rs refactoring (2,956 lines → smaller modules) - Maintains all existing APIs and functionality - no breaking changes ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [x] Refactoring (non-breaking change that improves code organization) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] Documentation update - [ ] Performance improvement - [ ] Test improvement ## Testing Checklist - [ ] Unit tests pass locally - [ ] Integration tests pass locally - [ ] Property-based tests pass locally - [ ] All examples compile and run successfully - [ ] Benchmarks show no performance regressions - [ ] Memory usage remains stable under load ## Performance Impact No performance impact expected - pure code organization change with identical runtime behavior. ## Security Checklist - [ ] Input validation requirements reviewed - [ ] Authentication/authorization impacts assessed - [ ] Data encryption requirements verified - [ ] Sensitive data exposure risks evaluated - [ ] Audit logging requirements confirmed ## Code Quality - [ ] Code follows established patterns and conventions - [ ] Complex logic is well-documented - [ ] Error handling is comprehensive - [ ] Resource cleanup is handled properly - [ ] Thread safety considerations addressed ## Reviewer Checklist - [ ] Business logic changes reviewed for correctness - [ ] Performance implications assessed - [ ] Security implications reviewed - [ ] Test coverage adequate for changes - [ ] Documentation updated as needed - [ ] Breaking changes properly communicated - [ ] Migration guide provided if needed ## Review Focus This is a pure refactoring with no functional changes. Focus on: - Verify module organization makes sense - Confirm no behavioral changes - Check that imports/exports are correct - Validate this follows the systematic refactoring plan from PLANNING.md 🤖 Generated with [Claude Code](https://claude.ai/code)

copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed

2025-07-05 13:54:34 -07:00

copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment

Pull Request Overview

This PR reorganizes the executor by extracting retry behavior into its own module, modularizing the core execution flow, and updating planning documentation to reflect refactoring priorities.

Extracted RetryConfig and RetryPolicy into executor/retry.rs
Split out stream discovery, execution, reading/writing, and metadata preparation into helper methods in executor.rs
Updated PLANNING.md with tooling instructions and next steps for systematic refactoring

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
eventcore/src/executor/retry.rs	New module housing retry configuration and policy
eventcore/src/executor.rs	Refactored execution loop into smaller functions
PLANNING.md	Adjusted refactoring plan and added workflow notes

Comments suppressed due to low confidence (3)

eventcore/src/executor.rs:748

[nitpick] Method naming is inconsistent: we have read_streams_with_timeout_and_circuit_breaker vs read_streams_with_circuit_breaker_only. Consider unifying these to a consistent pattern like read_streams_with_circuit_breaker and read_streams_with_timeout for clarity.

    async fn read_streams_with_timeout_and_circuit_breaker(

eventcore/src/executor/retry.rs:1

There are no unit tests covering RetryConfig::default or RetryPolicy::should_retry. Adding tests for each policy variant and default values would guard against regressions.

//! Retry logic and policies for command execution.

PLANNING.md:106

[nitpick] The instructions referring to Claude and enforcing a final 'move to next refactor' item are tooling‐specific and may confuse developers. Consider separating or clearly marking these workflow directives from the core planning content.

3. **Chain PRs** - Each subsequent PR branches off the previous one

## Pull Request Overview This PR reorganizes the executor by extracting retry behavior into its own module, modularizing the core execution flow, and updating planning documentation to reflect refactoring priorities. - Extracted `RetryConfig` and `RetryPolicy` into `executor/retry.rs` - Split out stream discovery, execution, reading/writing, and metadata preparation into helper methods in `executor.rs` - Updated `PLANNING.md` with tooling instructions and next steps for systematic refactoring ### Reviewed Changes Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments. | File | Description | |-----------------------------------------|---------------------------------------------------| | eventcore/src/executor/retry.rs | New module housing retry configuration and policy | | eventcore/src/executor.rs | Refactored execution loop into smaller functions | | PLANNING.md | Adjusted refactoring plan and added workflow notes| <details> <summary>Comments suppressed due to low confidence (3)</summary> **eventcore/src/executor.rs:748** * [nitpick] Method naming is inconsistent: we have `read_streams_with_timeout_and_circuit_breaker` vs `read_streams_with_circuit_breaker_only`. Consider unifying these to a consistent pattern like `read_streams_with_circuit_breaker` and `read_streams_with_timeout` for clarity. ``` async fn read_streams_with_timeout_and_circuit_breaker( ``` **eventcore/src/executor/retry.rs:1** * There are no unit tests covering `RetryConfig::default` or `RetryPolicy::should_retry`. Adding tests for each policy variant and default values would guard against regressions. ``` //! Retry logic and policies for command execution. ``` **PLANNING.md:106** * [nitpick] The instructions referring to Claude and enforcing a final 'move to next refactor' item are tooling‐specific and may confuse developers. Consider separating or clearly marking these workflow directives from the core planning content. ``` 3. **Chain PRs** - Each subsequent PR branches off the previous one ``` </details>

eventcore/src/executor.rs

The add_streams method increments the iteration counter and next_iteration also increments it, causing iteration to advance twice when new streams are discovered. Consider removing the increment from add_streams and handling all iteration counting in next_iteration.

    /// Adds newly discovered streams.
    fn add_streams(&mut self, new_streams: Vec<StreamId>) {
        self.stream_ids.extend(new_streams);

The add_streams method increments the iteration counter and next_iteration also increments it, causing iteration to advance twice when new streams are discovered. Consider removing the increment from add_streams and handling all iteration counting in next_iteration. ```suggestion /// Adds newly discovered streams. fn add_streams(&mut self, new_streams: Vec<StreamId>) { self.stream_ids.extend(new_streams); ```

eventcore/src/executor/retry.rs

					
				@ -0,0 +38,4 @@

				    /// Retry on concurrency conflicts and transient errors.

				    ConcurrencyAndTransient,

				    /// Custom policy with user-defined predicate.

				    Custom(fn(&CommandError) -> bool),

[nitpick] Using a bare function pointer for the Custom policy limits flexibility. Consider using Box<dyn Fn(&CommandError) -> bool + Send + Sync> so callers can supply closures with captured state.

#[derive(Debug)]
pub enum RetryPolicy {
    /// Only retry on concurrency conflicts.
    ConcurrencyConflictsOnly,
    /// Retry on concurrency conflicts and transient errors.
    ConcurrencyAndTransient,
    /// Custom policy with user-defined predicate.
    Custom(Box<dyn Fn(&CommandError) -> bool + Send + Sync>),

[nitpick] Using a bare function pointer for the `Custom` policy limits flexibility. Consider using `Box<dyn Fn(&CommandError) -> bool + Send + Sync>` so callers can supply closures with captured state. ```suggestion #[derive(Debug)] pub enum RetryPolicy { /// Only retry on concurrency conflicts. ConcurrencyConflictsOnly, /// Retry on concurrency conflicts and transient errors. ConcurrencyAndTransient, /// Custom policy with user-defined predicate. Custom(Box<dyn Fn(&CommandError) -> bool + Send + Sync>), ```

jwilger commented

2025-07-06 14:11:12 -07:00

(Migrated from github.com)

Closing this PR. While RetryConfig and RetryPolicy are still in executor.rs, the major functional refactoring in PR #10 has significantly changed the structure of the executor module. The benefit of extracting these small types is now minimal compared to the effort required to rebase and maintain this change.