Fix validation logic for manager indexer #2797
Conversation
Signed-off-by: aknishid <koco155@gmail.com>
📝 WalkthroughWalkthroughThe changes involve modifying the validation logic in the Changes
Sequence DiagramsequenceDiagram
participant Indexer
participant Configuration
Indexer->>Configuration: Validate Duration Parameters
alt Invalid Duration
Configuration-->>Indexer: Return Error if Any Duration < 0
else Valid Duration
Configuration-->>Indexer: Proceed with Initialization
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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
🧹 Nitpick comments (2)
pkg/manager/index/service/indexer.go (2)
79-81: The validation logic aligns with the system's behavior but needs documentation.The change correctly reflects the system's behavior where negative durations are converted to
MaxInt64in theStartmethod. However, this behavior should be documented to help future maintainers understand the validation logic.Add a comment explaining the validation logic:
+// Validate duration parameters. The validation only fails when all durations +// are negative since any negative duration is converted to MaxInt64 in Start(). if i.indexDuration < 0 && i.indexDurationLimit < 0 && i.saveIndexDurationLimit < 0 { return nil, errors.ErrInvalidConfig }
79-81: Consider moving duration normalization to New function.Currently, there's a disconnect between validation in
Newand normalization inStart. Consider moving the duration normalization logic toNewfor better encapsulation and to ensure the struct is fully initialized before use.Here's a suggested refactor:
func New(opts ...Option) (idx Indexer, err error) { i := new(index) for _, opt := range append(defaultOptions, opts...) { if err := opt(i); err != nil { return nil, errors.ErrOptionFailed(err, reflect.ValueOf(opt)) } } i.indexing.Store(false) i.saving.Store(false) if i.indexDuration < 0 && i.indexDurationLimit < 0 && i.saveIndexDurationLimit < 0 { return nil, errors.ErrInvalidConfig } + // Normalize negative durations to MaxInt64 + if i.indexDuration <= 0 { + i.indexDuration = math.MaxInt64 + } + if i.indexDurationLimit <= 0 { + i.indexDurationLimit = math.MaxInt64 + } + if i.saveIndexDuration <= 0 { + i.saveIndexDuration = math.MaxInt64 + } + if i.saveIndexDurationLimit <= 0 { + i.saveIndexDurationLimit = math.MaxInt64 + } return i, nil }Also applies to: 86-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/manager/index/service/indexer.go(1 hunks)
🔇 Additional comments (1)
pkg/manager/index/service/indexer.go (1)
79-81: Verify the validation logic across the codebase.Let's verify that this change in validation logic doesn't affect any other parts of the system that might depend on the previous behavior.
✅ Verification successful
Validation logic change is isolated and safe
The validation logic for duration fields is self-contained within the indexer.go file and doesn't affect other parts of the system. No other components directly validate these duration fields or depend on this specific validation behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any tests or code that might depend on the validation behavior rg -A 5 "ErrInvalidConfig.*indexDuration|indexDuration.*ErrInvalidConfig" # Search for any documentation or comments about the validation logic rg -l "indexDuration|indexDurationLimit|saveIndexDurationLimit" | xargs rg -l "validation|valid|invalid"Length of output: 219
Script:
#!/bin/bash # Search for test files rg -l "indexer_test.go" # Search for configuration structs and their usage ast-grep --pattern 'type $_ struct { $$$ indexDuration $_ $$$ }' # Search for ErrInvalidConfig usage patterns rg -A 3 "ErrInvalidConfig"Length of output: 13698
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2797 +/- ##
==========================================
- Coverage 23.93% 23.92% -0.01%
==========================================
Files 546 546
Lines 54555 54555
==========================================
- Hits 13058 13054 -4
- Misses 40712 40717 +5
+ Partials 785 784 -1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: aknishid <koco155@gmail.com>
aknishid
dismissed stale reviews from kpango and coderabbitai[bot]
via
ac793a5
|
Thanks @aknishid |
Description
It appears that the current validation does not account for cases where saveIndexDurationLimit might be set to a significantly large negative value (e.g., -1h). This could inadvertently result in valid configurations being flagged as errors.
Even when indexDuration, indexDurationLimit, and saveIndexDurationLimit are all set to negative values, the system assigns MaxInt64 at startup, ensuring that an index creation will eventually occur. Because of this, I wonder if this validation is strictly necessary.
If the goal is to detect only configurations where index duration is effectively disabled, we could modify the validation to:
This way, the validation triggers an error only when all three parameters are negative.
As I do not have full context on the original intent behind this validation, I would appreciate any insights or corrections if I am misunderstanding its purpose.
Related Issue
Versions
Checklist
Special notes for your reviewer
Summary by CodeRabbit