-
Notifications
You must be signed in to change notification settings - Fork 2
Apply TXM and LogPoller default config values field by field #302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apply TXM and LogPoller default config values field by field #302
Conversation
| MaxSendRetryAttempts: 5, | ||
| TxExpirationMins: 5, | ||
| CleanupIntervalMins: 60, | ||
| StickyNodeContextEnabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing StickyNodeContextEnabled as it doesn't seem to be used anywhere...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the flag is about keeping a consistent connection with the same lite server to avoid state difference between nodes, by default lite client implements connection pool(but we're only feeding one atm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool thanks, we can add it back when needed. Also would prefer to add it as StickyNodeContextDisabled since default/unset bool values are false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes the TXM and LogPoller configuration by changing from primitive types with mixed units to proper Duration types, and implements field-by-field default application. The key changes include migrating from uint fields representing different time units to consistent *config.Duration types and adding proper configuration validation and default application methods.
- Replaced TXM config fields with Duration types (
ConfirmPollSecs→ConfirmPollInterval, etc.) - Added
ApplyDefaults()andValidateConfig()methods to both TXM and LogPoller configs - Updated TOML configuration to apply defaults field-by-field rather than replacing entire config sections
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/txm/txm.go | Updated to use Duration methods instead of manual time conversions |
| pkg/txm/config.go | Added Duration fields, ApplyDefaults and ValidateConfig methods |
| pkg/txm/config_test.go | Added comprehensive tests for default application behavior |
| pkg/logpoller/config.go | Added ApplyDefaults and ValidateConfig methods |
| pkg/logpoller/config_test.go | Added tests for LogPoller default application |
| pkg/config/toml.go | Modified to apply defaults field-by-field and call validation hooks |
| pkg/config/toml_test.go | Updated tests to verify field-by-field default behavior |
| integration-tests/txm/txm_test.go | Updated to use new Duration-based config field |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jadepark-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 🎉
Durations instead ofuints that represent different unitsValidateConfigandApplyDefaultsin the TXM and logPoller config