Log progress. #270
Log progress. #270
Conversation
|
Warning Rate limit exceeded@ggreer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
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
🧹 Nitpick comments (3)
pkg/sync/syncer.go (3)
45-50: Add documentation for the Counts struct and its fields.Consider adding documentation to explain:
- The purpose of the struct
- What each field represents
- The expected usage patterns
Add this documentation above the struct:
+// Counts tracks various metrics during the synchronization process. +// It maintains counters for resource types, resources per type, +// and progress tracking for entitlements and grants. type Counts struct { + // ResourceTypes tracks the total number of resource types synced ResourceTypes int + // Resources maps resource type IDs to their count Resources map[string]int + // EntitlementsProgress tracks the progress of entitlement syncing per resource EntitlementsProgress map[string]int + // GrantsProgress tracks the progress of grant syncing per resource GrantsProgress map[string]int }
513-518: Add a safety check for map initialization.While the map is initialized in the constructor, it's a good practice to ensure the map entry exists before incrementing.
Consider this safer approach:
resourceTypeId := s.state.ResourceTypeID(ctx) +if _, ok := s.counts.Resources[resourceTypeId]; !ok { + s.counts.Resources[resourceTypeId] = 0 +} s.counts.Resources[resourceTypeId] += len(resp.List) if resp.NextPageToken == "" { l := ctxzap.Extract(ctx) l.Info("Synced resources", zap.String("resource_type_id", resourceTypeId), zap.Int("count", s.counts.Resources[resourceTypeId]))
48-49: Consider implementing EntitlementsProgress and GrantsProgress tracking.As mentioned in the PR description, tracking progress for entitlements and grants is planned. These fields are defined but not yet utilized in the respective sync functions (
SyncEntitlementsandSyncGrants).Consider similar patterns as used in
SyncResourceTypesandsyncResourceswhen implementing these:
- Initialize map entries before use
- Update counts after successful operations
- Log progress at appropriate points
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(5 hunks)
🔇 Additional comments (3)
pkg/sync/syncer.go (3)
65-65: LGTM!
The counts field is appropriately added to the syncer struct.
1616-1621: LGTM!
The initialization of the Counts struct is properly done with all maps initialized.
417-422: LGTM!
The resource types count is properly updated and logged.
…stuff we've done.
ggreer
force-pushed
the
ggreer/log-progress
branch
from
d9ddeb2 to
c3fe200
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/sync/syncer.go (1)
45-50: Consider adding documentation and thread safety.A few suggestions for improvement:
- Add documentation for the
Countsstruct to describe its purpose and usage.- Consider adding mutex protection for the count maps if there's any possibility of concurrent access.
Example documentation:
+// Counts tracks the progress of various sync operations including resource types, +// resources, entitlements, and grants. It provides visibility into the sync process +// and helps monitor the amount of work completed. type Counts struct { ResourceTypes int Resources map[string]int EntitlementsProgress map[string]int GrantsProgress map[string]int }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(5 hunks)
🔇 Additional comments (3)
pkg/sync/syncer.go (3)
45-50: LGTM! Well-structured count tracking.
The Counts struct is well-designed for tracking sync progress with appropriate field types for different counting needs.
417-422: LGTM! Effective progress tracking for resource types.
The implementation correctly tracks and logs the count of synced resource types.
1616-1621: LGTM! Proper initialization of count tracking.
The Counts struct is properly initialized with all required maps in the constructor.
pkg/sync/syncer.go
Outdated
| resourceTypeId := s.state.ResourceTypeID(ctx) | ||
| s.counts.Resources[resourceTypeId] += len(resp.List) | ||
|
|
||
| if resp.NextPageToken == "" { | ||
| l := ctxzap.Extract(ctx) | ||
| l.Info("Synced resources", zap.String("resource_type_id", resourceTypeId), zap.Int("count", s.counts.Resources[resourceTypeId])) |
There was a problem hiding this comment.
Protect against nil map panic.
The code assumes s.counts.Resources is initialized. While it's currently initialized in NewSyncer, it's better to ensure safety at the point of use.
Add a nil check before incrementing the count:
resourceTypeId := s.state.ResourceTypeID(ctx)
+if s.counts.Resources == nil {
+ s.counts.Resources = make(map[string]int)
+}
s.counts.Resources[resourceTypeId] += len(resp.List)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resourceTypeId := s.state.ResourceTypeID(ctx) | |
| s.counts.Resources[resourceTypeId] += len(resp.List) | |
| if resp.NextPageToken == "" { | |
| l := ctxzap.Extract(ctx) | |
| l.Info("Synced resources", zap.String("resource_type_id", resourceTypeId), zap.Int("count", s.counts.Resources[resourceTypeId])) | |
| resourceTypeId := s.state.ResourceTypeID(ctx) | |
| if s.counts.Resources == nil { | |
| s.counts.Resources = make(map[string]int) | |
| } | |
| s.counts.Resources[resourceTypeId] += len(resp.List) | |
| if resp.NextPageToken == "" { | |
| l := ctxzap.Extract(ctx) | |
| l.Info("Synced resources", zap.String("resource_type_id", resourceTypeId), zap.Int("count", s.counts.Resources[resourceTypeId])) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/sync/syncer.go (3)
45-52: Add documentation for the ProgressCounts struct.The struct is well-designed for tracking sync progress, but would benefit from documentation explaining its purpose and the significance of each field.
Add documentation like this:
+// ProgressCounts tracks the progress of synchronization operations across different +// resource types. It maintains counts of processed resource types, resources, +// entitlements, and grants, along with timestamps to control logging frequency. type ProgressCounts struct { + // ResourceTypes tracks the total number of resource types processed ResourceTypes int + // Resources maps resource type IDs to their processed resource count Resources map[string]int + // EntitlementsProgress maps resource type IDs to their processed entitlement count EntitlementsProgress map[string]int + // LastEntitlementLog tracks when entitlement progress was last logged LastEntitlementLog time.Time + // GrantsProgress maps resource type IDs to their processed grant count GrantsProgress map[string]int + // LastGrantLog tracks when grant progress was last logged LastGrantLog time.Time }
65-67: Remove redundant TODO comment.The code already implements the described functionality. The TODO comment about logging progress based on completion or time interval is already implemented in the switch statement below.
- // TODO: only log progress if we complete or we haven't logged in the past 10 seconds l := ctxzap.Extract(ctx)
1524-1525: Extract magic numbers into named constants.The logging condition uses magic numbers (250 and 10) which would be clearer as named constants.
+const ( + grantExpansionLogInterval = 250 + grantExpansionInitialLogThreshold = 10 +) -if actions%250 == 0 || actions <= 10 { +if actions%grantExpansionLogInterval == 0 || actions <= grantExpansionInitialLogThreshold {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(8 hunks)
🔇 Additional comments (2)
pkg/sync/syncer.go (2)
586-588: Protect against nil map panic.
The code assumes s.counts.Resources is initialized. While it's currently initialized in NewSyncer, it's better to ensure safety at the point of use.
1693-1698: LGTM: Proper initialization of ProgressCounts.
The initialization is complete with all maps properly initialized to prevent nil map panics.
pkg/sync/syncer.go
Outdated
| resources := p.Resources[resourceType] | ||
| percentComplete := (entitlementsProgress * 100) / resources | ||
|
|
There was a problem hiding this comment.
Add protection against division by zero.
Both LogEntitlementsProgress and LogGrantsProgress calculate percentages without checking if the resources count is zero.
Add zero checks before calculating percentages:
resources := p.Resources[resourceType]
+if resources == 0 {
+ return
+}
percentComplete := (entitlementsProgress * 100) / resourcesAlso applies to: 98-100
…ncing. Use better names for counts.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/sync/syncer.go (4)
45-52: Add documentation for struct fields.The
ProgressCountsstruct fields lack documentation explaining their purpose and usage. Consider adding field-level documentation to improve code maintainability.type ProgressCounts struct { + // ResourceTypes tracks the total number of resource types synced ResourceTypes int + // Resources maps resource type IDs to their count of synced resources Resources map[string]int + // EntitlementsProgress maps resource type IDs to their count of processed entitlements EntitlementsProgress map[string]int + // LastEntitlementLog maps resource type IDs to the timestamp of their last entitlement log LastEntitlementLog map[string]time.Time + // GrantsProgress maps resource type IDs to their count of processed grants GrantsProgress map[string]int + // LastGrantLog maps resource type IDs to the timestamp of their last grant log LastGrantLog map[string]time.Time }
54-63: Address thread safety concerns.The TODO comment indicates potential thread safety issues when the code becomes parallel. Consider implementing thread safety now to prevent future issues.
Consider using sync.Map or adding mutex protection:
type ProgressCounts struct { + mu sync.RWMutex ResourceTypes int Resources map[string]int EntitlementsProgress map[string]int LastEntitlementLog map[string]time.Time GrantsProgress map[string]int LastGrantLog map[string]time.Time } func NewProgressCounts() *ProgressCounts { + p := &ProgressCounts{ + mu: sync.RWMutex{}, Resources: make(map[string]int), EntitlementsProgress: make(map[string]int), LastEntitlementLog: make(map[string]time.Time), GrantsProgress: make(map[string]int), LastGrantLog: make(map[string]time.Time), } + return p }
108-137: Refactor to reduce code duplication with LogEntitlementsProgress.The LogGrantsProgress method is nearly identical to LogEntitlementsProgress. Consider refactoring to reduce duplication.
+type progressType struct { + name string + progress map[string]int + lastLog map[string]time.Time + logComplete func(l *zap.Logger, resourceType string, count, total int) + logInProgress func(l *zap.Logger, resourceType string, count, total, percent int) + logError func(l *zap.Logger, resourceType string, count, total int) +} + +func (p *ProgressCounts) logProgress(ctx context.Context, resourceType string, pt progressType) { + l := ctxzap.Extract(ctx) + progress := pt.progress[resourceType] + resources := p.Resources[resourceType] + + if resources == 0 { + l.Debug("No resources to process", + zap.String("type", pt.name), + zap.String("resource_type_id", resourceType)) + return + } + + percentComplete := (progress * 100) / resources + + switch { + case progress > resources: + pt.logError(l, resourceType, progress, resources) + case percentComplete == 100: + pt.logComplete(l, resourceType, progress, resources) + pt.lastLog[resourceType] = time.Time{} + case time.Since(pt.lastLog[resourceType]) > defaultProgressLogInterval: + pt.logInProgress(l, resourceType, progress, resources, percentComplete) + pt.lastLog[resourceType] = time.Now() + } +}
1537-1538: Consider making the progress logging threshold configurable.The hardcoded values for progress logging (250 and 10) should be configurable constants.
+const ( + expandGrantsLogThreshold = 250 + expandGrantsMinLogThreshold = 10 +) + - if actions%250 == 0 || actions <= 10 { + if actions%expandGrantsLogThreshold == 0 || actions <= expandGrantsMinLogThreshold { l.Info("Expanding grants", zap.Int("actions_remaining", actions)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(8 hunks)
🔇 Additional comments (1)
pkg/sync/syncer.go (1)
76-106: 🛠️ Refactor suggestion
Protect against division by zero and make logging interval configurable.
The method has two issues:
- Potential division by zero when calculating percentComplete
- Hardcoded 10-second logging interval
+const defaultProgressLogInterval = 10 * time.Second
+
func (p *ProgressCounts) LogEntitlementsProgress(ctx context.Context, resourceType string) {
l := ctxzap.Extract(ctx)
entitlementsProgress := p.EntitlementsProgress[resourceType]
resources := p.Resources[resourceType]
+ if resources == 0 {
+ l.Debug("No resources to process for entitlements",
+ zap.String("resource_type_id", resourceType))
+ return
+ }
percentComplete := (entitlementsProgress * 100) / resources
switch {
case entitlementsProgress > resources:
// ... existing code ...
- case time.Since(p.LastEntitlementLog[resourceType]) > 10*time.Second:
+ case time.Since(p.LastEntitlementLog[resourceType]) > defaultProgressLogInterval:
// ... existing code ...
}
}Likely invalid or redundant comment.
| s.counts.ResourceTypes += len(resp.List) | ||
| s.handleProgress(ctx, s.state.Current(), len(resp.List)) | ||
|
|
||
| if resp.NextPageToken == "" { | ||
| s.counts.LogResourceTypesProgress(ctx) | ||
| s.state.FinishAction(ctx) | ||
| return nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for map operations.
The ResourceTypes counter is incremented without any error handling, and the map operations need to be thread-safe.
+ p.mu.Lock()
s.counts.ResourceTypes += len(resp.List)
+ p.mu.Unlock()
s.handleProgress(ctx, s.state.Current(), len(resp.List))
if resp.NextPageToken == "" {
+ p.mu.RLock()
s.counts.LogResourceTypesProgress(ctx)
+ p.mu.RUnlock()
s.state.FinishAction(ctx)
return nil
}Committable suggestion skipped: line range outside the PR's diff.
| resourceTypeId := s.state.ResourceTypeID(ctx) | ||
| s.counts.Resources[resourceTypeId] += len(resp.List) | ||
|
|
||
| if resp.NextPageToken == "" { | ||
| s.counts.LogResourcesProgress(ctx, resourceTypeId) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add thread-safe map access and error handling.
The Resources map is accessed without thread safety and error handling.
resourceTypeId := s.state.ResourceTypeID(ctx)
+ s.counts.mu.Lock()
+ if s.counts.Resources == nil {
+ s.counts.Resources = make(map[string]int)
+ }
s.counts.Resources[resourceTypeId] += len(resp.List)
+ s.counts.mu.Unlock()
if resp.NextPageToken == "" {
+ s.counts.mu.RLock()
s.counts.LogResourcesProgress(ctx, resourceTypeId)
+ s.counts.mu.RUnlock()
s.state.FinishAction(ctx)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resourceTypeId := s.state.ResourceTypeID(ctx) | |
| s.counts.Resources[resourceTypeId] += len(resp.List) | |
| if resp.NextPageToken == "" { | |
| s.counts.LogResourcesProgress(ctx, resourceTypeId) | |
| resourceTypeId := s.state.ResourceTypeID(ctx) | |
| s.counts.mu.Lock() | |
| if s.counts.Resources == nil { | |
| s.counts.Resources = make(map[string]int) | |
| } | |
| s.counts.Resources[resourceTypeId] += len(resp.List) | |
| s.counts.mu.Unlock() | |
| if resp.NextPageToken == "" { | |
| s.counts.mu.RLock() | |
| s.counts.LogResourcesProgress(ctx, resourceTypeId) | |
| s.counts.mu.RUnlock() | |
| s.state.FinishAction(ctx) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/sync/syncer.go (3)
76-109: Address the TODO comment and consider extracting common logging logic.The TODO comment about logging frequency is already implemented. However, the logging logic is duplicated between entitlements and grants progress.
Consider extracting the common logging logic into a helper function:
+func (p *ProgressCounts) logProgress(ctx context.Context, resourceType string, synced int, total int, lastLog map[string]time.Time, logType string) { + if total == 0 { + return + } + percentComplete := (synced * 100) / total + + l := ctxzap.Extract(ctx) + switch { + case synced > total: + l.Error(fmt.Sprintf("more %s than resources", logType), + zap.String("resource_type_id", resourceType), + zap.Int("synced", synced), + zap.Int("total", total), + ) + case percentComplete == 100: + l.Info(fmt.Sprintf("Synced %s", logType), + zap.String("resource_type_id", resourceType), + zap.Int("count", synced), + zap.Int("total", total), + ) + lastLog[resourceType] = time.Time{} + case time.Since(lastLog[resourceType]) > 10*time.Second: + l.Info(fmt.Sprintf("Syncing %s", logType), + zap.String("resource_type_id", resourceType), + zap.Int("synced", synced), + zap.Int("total", total), + zap.Int("percent_complete", percentComplete), + ) + lastLog[resourceType] = time.Now() + } +}Then use it in both methods:
func (p *ProgressCounts) LogEntitlementsProgress(ctx context.Context, resourceType string) { - // TODO: only log progress if we complete or we haven't logged in the past 10 seconds entitlementsProgress := p.EntitlementsProgress[resourceType] resources := p.Resources[resourceType] - if resources == 0 { - return - } - percentComplete := (entitlementsProgress * 100) / resources - ... + p.logProgress(ctx, resourceType, entitlementsProgress, resources, p.LastEntitlementLog, "entitlements") }
1543-1544: Consider consistent progress logging for grant expansion.The grant expansion progress logging uses a different pattern than other progress logging methods.
Consider using the same pattern as other progress logging methods for consistency:
-if actions%250 == 0 || actions <= 10 { - l.Info("Expanding grants", zap.Int("actions_remaining", actions)) +if actions == 0 { + l.Info("Starting grant expansion") +} else if actions%250 == 0 || actions <= 10 { + l.Info("Expanding grants", + zap.Int("actions_remaining", actions), + zap.Int("percent_complete", ((total_actions - actions) * 100) / total_actions)) }
1712-1712: Consider initializing counts in a separate method.The counts initialization is mixed with other syncer initialization logic.
Consider extracting the initialization into a separate method for better separation of concerns:
+func (s *syncer) initCounts() { + s.counts = NewProgressCounts() +} + func NewSyncer(ctx context.Context, c types.ConnectorClient, opts ...SyncOpt) (Syncer, error) { s := &syncer{ connector: c, skipEGForResourceType: make(map[string]bool), - counts: NewProgressCounts(), } + s.initCounts()
pkg/sync/syncer.go
Outdated
| type ProgressCounts struct { | ||
| ResourceTypes int | ||
| Resources map[string]int | ||
| EntitlementsProgress map[string]int | ||
| LastEntitlementLog map[string]time.Time | ||
| GrantsProgress map[string]int | ||
| LastGrantLog map[string]time.Time | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field documentation and consider thread safety.
The struct fields need documentation to explain their purpose. Additionally, consider adding a mutex for thread-safe operations.
type ProgressCounts struct {
+ // mu protects concurrent access to all fields
+ mu sync.RWMutex
+ // ResourceTypes tracks the total number of resource types processed
ResourceTypes int
+ // Resources maps resource type IDs to their count
Resources map[string]int
+ // EntitlementsProgress maps resource type IDs to their entitlements progress
EntitlementsProgress map[string]int
+ // LastEntitlementLog tracks the last time entitlements progress was logged
LastEntitlementLog map[string]time.Time
+ // GrantsProgress maps resource type IDs to their grants progress
GrantsProgress map[string]int
+ // LastGrantLog tracks the last time grants progress was logged
LastGrantLog map[string]time.Time
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ProgressCounts struct { | |
| ResourceTypes int | |
| Resources map[string]int | |
| EntitlementsProgress map[string]int | |
| LastEntitlementLog map[string]time.Time | |
| GrantsProgress map[string]int | |
| LastGrantLog map[string]time.Time | |
| } | |
| type ProgressCounts struct { | |
| // mu protects concurrent access to all fields | |
| mu sync.RWMutex | |
| // ResourceTypes tracks the total number of resource types processed | |
| ResourceTypes int | |
| // Resources maps resource type IDs to their count | |
| Resources map[string]int | |
| // EntitlementsProgress maps resource type IDs to their entitlements progress | |
| EntitlementsProgress map[string]int | |
| // LastEntitlementLog tracks the last time entitlements progress was logged | |
| LastEntitlementLog map[string]time.Time | |
| // GrantsProgress maps resource type IDs to their grants progress | |
| GrantsProgress map[string]int | |
| // LastGrantLog tracks the last time grants progress was logged | |
| LastGrantLog map[string]time.Time | |
| } |
pkg/sync/syncer.go
Outdated
| // TODO: use a mutex or a syncmap for when this code becomes parallel | ||
| func NewProgressCounts() *ProgressCounts { | ||
| return &ProgressCounts{ | ||
| Resources: make(map[string]int), | ||
| EntitlementsProgress: make(map[string]int), | ||
| LastEntitlementLog: make(map[string]time.Time), | ||
| GrantsProgress: make(map[string]int), | ||
| LastGrantLog: make(map[string]time.Time), | ||
| } | ||
| } |
There was a problem hiding this comment.
Address the TODO comment about thread safety.
The TODO comment indicates that the code will become parallel, but the initialization doesn't include thread safety mechanisms.
-// TODO: use a mutex or a syncmap for when this code becomes parallel
func NewProgressCounts() *ProgressCounts {
return &ProgressCounts{
+ mu: sync.RWMutex{},
Resources: make(map[string]int),
EntitlementsProgress: make(map[string]int),
LastEntitlementLog: make(map[string]time.Time),
GrantsProgress: make(map[string]int),
LastGrantLog: make(map[string]time.Time),
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/sync/syncer.go (2)
524-530:⚠️ Potential issueAdd thread safety for resource type counting.
The ResourceTypes counter needs mutex protection.
Apply this diff:
+ s.counts.mu.Lock() s.counts.ResourceTypes += len(resp.List) + s.counts.mu.Unlock() s.handleProgress(ctx, s.state.Current(), len(resp.List)) if resp.NextPageToken == "" { + s.counts.mu.RLock() s.counts.LogResourceTypesProgress(ctx) + s.counts.mu.RUnlock() s.state.FinishAction(ctx) return nil }
619-623:⚠️ Potential issueProtect Resources map access.
The Resources map needs mutex protection and nil check.
Apply this diff:
resourceTypeId := s.state.ResourceTypeID(ctx) + s.counts.mu.Lock() + if s.counts.Resources == nil { + s.counts.Resources = make(map[string]int) + } s.counts.Resources[resourceTypeId] += len(resp.List) + s.counts.mu.Unlock() if resp.NextPageToken == "" { + s.counts.mu.RLock() s.counts.LogResourcesProgress(ctx, resourceTypeId) + s.counts.mu.RUnlock() s.state.FinishAction(ctx) }
🧹 Nitpick comments (1)
pkg/sync/syncer.go (1)
172-172: Consider cleanup of ProgressCounts.The
countsfield is properly initialized, but consider adding cleanup in theClosemethod if any resources need to be released.Also applies to: 1723-1723
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/sync/syncer.go(8 hunks)
🔇 Additional comments (3)
pkg/sync/syncer.go (3)
1358-1359:
Add thread safety for grants progress.
The GrantsProgress counter needs mutex protection.
Apply this diff:
+ s.counts.mu.Lock()
s.counts.GrantsProgress[resourceID.ResourceType] += 1
+ s.counts.mu.Unlock()
+ s.counts.mu.RLock()
s.counts.LogGrantsProgress(ctx, resourceID.ResourceType)
+ s.counts.mu.RUnlock()Likely invalid or redundant comment.
57-67:
Address thread safety in initialization.
The TODO comment indicates future parallelism needs, but initialization doesn't include thread safety mechanisms.
Apply this diff:
-// TODO: use a mutex or a syncmap for when this code becomes parallel
func NewProgressCounts() *ProgressCounts {
return &ProgressCounts{
+ mu: sync.RWMutex{},
Resources: make(map[string]int),
EntitlementsProgress: make(map[string]int),
LastEntitlementLog: make(map[string]time.Time),
GrantsProgress: make(map[string]int),
LastGrantLog: make(map[string]time.Time),
LastActionLog: time.Time{},
}
}Likely invalid or redundant comment.
45-53: 🛠️ Refactor suggestion
Add field documentation and mutex for thread safety.
The struct fields need documentation and thread safety protection.
Apply this diff:
type ProgressCounts struct {
+ // mu protects concurrent access to all fields
+ mu sync.RWMutex
+ // ResourceTypes tracks the total number of resource types processed
ResourceTypes int
+ // Resources maps resource type IDs to their count
Resources map[string]int
+ // EntitlementsProgress maps resource type IDs to their entitlements progress
EntitlementsProgress map[string]int
+ // LastEntitlementLog tracks the last time entitlements progress was logged
LastEntitlementLog map[string]time.Time
+ // GrantsProgress maps resource type IDs to their grants progress
GrantsProgress map[string]int
+ // LastGrantLog tracks the last time grants progress was logged
LastGrantLog map[string]time.Time
+ // LastActionLog tracks the last time expand progress was logged
LastActionLog time.Time
}Likely invalid or redundant comment.
| s.counts.EntitlementsProgress[resourceID.ResourceType] += 1 | ||
| s.counts.LogEntitlementsProgress(ctx, resourceID.ResourceType) | ||
|
|
There was a problem hiding this comment.
Add thread safety for entitlements progress.
The EntitlementsProgress counter needs mutex protection.
Apply this diff:
+ s.counts.mu.Lock()
s.counts.EntitlementsProgress[resourceID.ResourceType] += 1
+ s.counts.mu.Unlock()
+ s.counts.mu.RLock()
s.counts.LogEntitlementsProgress(ctx, resourceID.ResourceType)
+ s.counts.mu.RUnlock()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.counts.EntitlementsProgress[resourceID.ResourceType] += 1 | |
| s.counts.LogEntitlementsProgress(ctx, resourceID.ResourceType) | |
| s.counts.mu.Lock() | |
| s.counts.EntitlementsProgress[resourceID.ResourceType] += 1 | |
| s.counts.mu.Unlock() | |
| s.counts.mu.RLock() | |
| s.counts.LogEntitlementsProgress(ctx, resourceID.ResourceType) | |
| s.counts.mu.RUnlock() | |
| if actions%250 == 0 || actions < 10 { | ||
| l.Info("Expanding grants", zap.Int("actions", actions)) | ||
| } | ||
| s.counts.LogExpandProgress(ctx, graph.Actions) |
There was a problem hiding this comment.
Add thread safety for expand progress logging.
The LogExpandProgress call needs read lock protection.
Apply this diff:
+ s.counts.mu.RLock()
s.counts.LogExpandProgress(ctx, graph.Actions)
+ s.counts.mu.RUnlock()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.counts.LogExpandProgress(ctx, graph.Actions) | |
| s.counts.mu.RLock() | |
| s.counts.LogExpandProgress(ctx, graph.Actions) | |
| s.counts.mu.RUnlock() |
…. Just log every 10 seconds max.
This implementation will probably change in the future, as we'll want to make it safe for concurrent actions.
Summary by CodeRabbit
Summary by CodeRabbit