feat: bulk checker goroutine cancellation #1694
Conversation
…mprove concurrency handling
WalkthroughThe pull request includes multiple updates primarily focused on enhancing the structure and readability of configuration files and code within the Permify project. Key modifications involve changing the Changes
Possibly related PRs
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: 5
🧹 Outside diff range and nitpick comments (4)
internal/servers/dataServer.go (2)
66-70: LGTM! Consider extracting the default page size as a constant.The changes improve the robustness of the pagination logic by ensuring a default page size when not specified by the client. This is a good practice for maintaining consistent behavior.
For better code maintainability and consistency, consider extracting the default page size (50) as a package-level constant. This would allow easy modification of the default value across all methods if needed in the future. For example:
const defaultPageSize = 50 // In the method size := request.GetPageSize() if size == 0 { size = defaultPageSize }Also applies to: 91-91
Line range hint
1-442: Overall, the changes improve pagination handling.The modifications to
ReadRelationshipsandReadAttributesmethods enhance the robustness of pagination logic by ensuring a default page size when not specified by the client. These changes are consistent across both methods and do not introduce any new issues.To further improve the code:
- Extract the default page size as a package-level constant for better maintainability.
- Consider adding a unit test to verify the behavior when the client doesn't specify a page size, ensuring the default is correctly applied.
internal/engines/utils.go (1)
85-85: LGTM. Consider adding documentation for the new field.The addition of
ea sync.Mapis consistent with the existing structure and extends the functionality ofVisitsMap. To improve clarity, consider adding a comment explaining the purpose of this new field.type VisitsMap struct { er sync.Map - ea sync.Map + ea sync.Map // Stores entity-attribute pairs published sync.Map }internal/engines/bulk.go (1)
134-134: Usecopyinstead ofappendto duplicate the request listUsing
copycan be more efficient and idiomatic when duplicating slices.Apply this diff:
- return append([]BulkCheckerRequest{}, bc.list...) // Return a copy of the list to avoid modifying the original + listCopy := make([]BulkCheckerRequest, len(bc.list)) + copy(listCopy, bc.list) + return listCopy // Return a copy of the list to avoid modifying the original
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
- .goreleaser.yml (1 hunks)
- docs/api-reference/apidocs.swagger.json (1 hunks)
- docs/api-reference/openapiv2/apidocs.swagger.json (1 hunks)
- internal/engines/bulk.go (7 hunks)
- internal/engines/entityFilter.go (2 hunks)
- internal/engines/utils.go (2 hunks)
- internal/info.go (1 hunks)
- internal/servers/dataServer.go (4 hunks)
- proto/base/v1/openapi.proto (1 hunks)
✅ Files skipped from review due to trivial changes (3)
- docs/api-reference/apidocs.swagger.json
- docs/api-reference/openapiv2/apidocs.swagger.json
- proto/base/v1/openapi.proto
🧰 Additional context used
🔇 Additional comments (7)
internal/info.go (1)
26-26: LGTM: Version increment looks good.The version has been correctly updated from "v1.1.3" to "v1.1.4", which is consistent with semantic versioning for a patch release.
To ensure consistency across the project, let's verify if this version update is reflected in other relevant files:
Please review the output of this script and update any necessary files to reflect the new version.
.goreleaser.yml (1)
Line range hint
1-96: Overall assessment of .goreleaser.yml changesThe change in this file is minimal but potentially significant. The modification from
name_templatetoversion_templatein thesnapshotsection may reflect an update in GoReleaser's configuration schema.While this change appears intentional and isolated, it's crucial to verify its correctness against the latest GoReleaser documentation. The verification script provided in the previous comment will help ensure this change aligns with current best practices.
Additionally, it's worth noting that no other parts of the file were modified. The overall structure and other configurations remain unchanged, which suggests this update was targeted specifically at the snapshot versioning mechanism.
internal/servers/dataServer.go (1)
117-121: LGTM! Consider the same improvement as in ReadRelationships.The changes in this method mirror those in
ReadRelationships, which is good for consistency across the codebase. They improve the robustness of the pagination logic in the same manner.As suggested for
ReadRelationships, consider extracting the default page size (50) as a package-level constant for better maintainability and consistency across both methods.Also applies to: 142-142
internal/engines/utils.go (1)
Line range hint
1-100: Summary of changes and recommendations
- The addition of the
ea sync.Mapfield to theVisitsMapstruct enhances its functionality to track entity-attribute pairs.- The new
AddEAmethod provides a way to add entity-attribute pairs to theVisitsMap.- A minor issue was found in the
AddEAmethod where it uses the wrong map (erinstead ofea).Please address the issue in the
AddEAmethod and consider adding documentation for the neweafield. Once these changes are made, the modifications will improve theVisitsMapfunctionality without introducing any breaking changes to the existing codebase.internal/engines/bulk.go (1)
215-221: EnsureIsContextRelatedErrorfunction is used correctlyAfter fixing
IsContextRelatedError, it will correctly identify context-related errors. Ensure that all places where it's called pass the appropriate error.Run the following script to check for all usages of
IsContextRelatedErrorand verify that the correct error variable is passed:✅ Verification successful
Usage of
IsContextRelatedErroris correct across all instances.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify correct usage of IsContextRelatedError # Search for all calls to IsContextRelatedError in Go files rg --type go 'IsContextRelatedError\([^)]+\)' -A 1Length of output: 1060
internal/engines/entityFilter.go (2)
127-129: Prevent redundant processing withvisits.AddEAGood job adding the check to prevent processing entities that have already been visited. This enhances efficiency and avoids infinite loops.
186-210: Efficiently publish unique entitiesThe added logic correctly checks for previously visited entities using
visits.AddPublished(entity), ensuring that duplicates are not processed. This maintains the integrity of the published results.
| @@ -93,7 +93,7 @@ nfpms: | |||
| maintainer: "permify <help@permify.co>" | |||
| vendor: "permify inc." | |||
| snapshot: | |||
| name_template: "{{ incpatch .Version }}-next" | |||
| version_template: "{{ incpatch .Version }}-next" | |||
There was a problem hiding this comment.
💡 Codebase verification
Issues with the change from name_template to version_template
The version_template key was not found in GoReleaser's documentation for snapshot configuration. Additionally, name_template is still present in .goreleaser.yml. Please verify and correct the configuration to ensure it aligns with GoReleaser's requirements.
🔗 Analysis chain
Confirm the change from name_template to version_template
The change from name_template to version_template in the snapshot section appears to be intentional. This modification alters how versioning for snapshots is defined.
To ensure this change aligns with GoReleaser's current best practices, please run the following verification script:
This script will help verify if the change is consistent with GoReleaser's current documentation and if all relevant occurrences have been updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'version_template' is the correct key for snapshot configuration in GoReleaser
# Test: Check GoReleaser's documentation for the correct key in snapshot configuration
curl -s https://goreleaser.com/customization/snapshot/ | grep -q 'version_template:'
if [ $? -eq 0 ]; then
echo "The change to 'version_template' is correct according to GoReleaser's documentation."
else
echo "Warning: 'version_template' was not found in GoReleaser's documentation for snapshot configuration. Please double-check this change."
fi
# Test: Check for any remaining occurrences of 'name_template' in the snapshot section
if grep -q 'name_template' .goreleaser.yml; then
echo "Warning: 'name_template' is still present in .goreleaser.yml. Ensure all occurrences have been updated if intended."
else
echo "All occurrences of 'name_template' have been updated in .goreleaser.yml."
fi
Length of output: 673
| func (s *VisitsMap) AddEA(entityType, attribute string) bool { | ||
| key := fmt.Sprintf("%s$%s", entityType, attribute) | ||
| _, existed := s.er.LoadOrStore(key, struct{}{}) | ||
| return !existed | ||
| } |
There was a problem hiding this comment.
Fix the map used in AddEA method.
The AddEA method is using the er map instead of the newly added ea map. This appears to be an error and should be corrected to ensure proper functionality.
Please apply the following fix:
func (s *VisitsMap) AddEA(entityType, attribute string) bool {
key := fmt.Sprintf("%s$%s", entityType, attribute)
- _, existed := s.er.LoadOrStore(key, struct{}{})
+ _, existed := s.ea.LoadOrStore(key, struct{}{})
return !existed
}📝 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.
| func (s *VisitsMap) AddEA(entityType, attribute string) bool { | |
| key := fmt.Sprintf("%s$%s", entityType, attribute) | |
| _, existed := s.er.LoadOrStore(key, struct{}{}) | |
| return !existed | |
| } | |
| func (s *VisitsMap) AddEA(entityType, attribute string) bool { | |
| key := fmt.Sprintf("%s$%s", entityType, attribute) | |
| _, existed := s.ea.LoadOrStore(key, struct{}{}) | |
| return !existed | |
| } |
| "github.com/pkg/errors" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the standard errors package
Since Go 1.13, the standard library's errors package provides errors.Is and errors.As functions. You may not need to import "github.com/pkg/errors".
Apply this diff to switch to the standard library:
- "github.com/pkg/errors"
+ "errors"📝 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.
| "github.com/pkg/errors" | |
| "errors" | |
| func IsContextRelatedError(ctx context.Context, err error) bool { | ||
| return errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) | ||
| } |
There was a problem hiding this comment.
Fix IsContextRelatedError to check the passed error
The function currently checks ctx.Err() instead of the provided err. It should check err to accurately determine if the error is context-related.
Apply this diff:
func IsContextRelatedError(ctx context.Context, err error) bool {
- return errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded)
+ return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)
}📝 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.
| func IsContextRelatedError(ctx context.Context, err error) bool { | |
| return errors.Is(ctx.Err(), context.Canceled) || errors.Is(ctx.Err(), context.DeadlineExceeded) | |
| } | |
| func IsContextRelatedError(ctx context.Context, err error) bool { | |
| return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) | |
| } |
| if err := ctx.Err(); err == context.Canceled { | ||
| return nil // Gracefully exit the goroutine if context is canceled | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle all context-related errors when checking context
Currently, only context.Canceled is checked. Consider handling context.DeadlineExceeded as well to cover all context cancellation scenarios.
Apply this diff:
- if err := ctx.Err(); err == context.Canceled {
+ if err := ctx.Err(); err != nil {
return nil // Gracefully exit the goroutine if context is canceled or deadline exceeded
}Alternatively, use the IsContextRelatedError function:
- if err := ctx.Err(); err == context.Canceled {
+ if IsContextRelatedError(ctx, ctx.Err()) {
return nil // Gracefully exit the goroutine if context is canceled or deadline exceeded
}📝 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.
| if err := ctx.Err(); err == context.Canceled { | |
| return nil // Gracefully exit the goroutine if context is canceled | |
| } | |
| if err := ctx.Err(); err != nil { | |
| return nil // Gracefully exit the goroutine if context is canceled or deadline exceeded | |
| } | |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores