feat: Optimize migrations #603
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily enhancing configuration handling and error management. In 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: 2
🧹 Outside diff range and nitpick comments (5)
deployments/pulumi/pkg/component.go (2)
159-160: Validation ofupgradeModevaluesThe validation correctly checks for invalid
upgradeModevalues. Consider using a whitelist or mapping to make it more maintainable as new modes are added.
Line range hint
517-553: IncorrectRestartPolicyvalue in Job specificationThe
RestartPolicyfor a Kubernetes Job must be either"OnFailure"or"Never". Using"Always"is invalid and will cause deployment errors.Update the
RestartPolicyto"OnFailure"to comply with Kubernetes specifications:RestartPolicy: pulumi.String("Always"), + RestartPolicy: pulumi.String("OnFailure"),deployments/pulumi/main.go (2)
43-43: Handle missingimage.pullPolicyconfiguration with a default valueWhen
image.pullPolicyis not set in the configuration,conf.Getreturns an empty string, which may not be the desired behavior. Assign a default value to ensure consistent deployment.ImagePullPolicy: pulumi.String(conf.Get("image.pullPolicy")), +// Set default pull policy if not provided +imagePullPolicy := conf.Get("image.pullPolicy") +if imagePullPolicy == "" { + imagePullPolicy = "IfNotPresent" // or another appropriate default +} +ImagePullPolicy: pulumi.String(imagePullPolicy),
47-49: Provide default values for optional configurationsUsing
conf.GetBoolandconf.GetIntwithout default values may lead to zero-values (false, 0) if the configuration is missing, which might not be intended.Consider specifying default values to ensure expected behavior:
Debug: pulumi.Bool(conf.GetBool("debug")), ReplicaCount: pulumi.Int(conf.GetInt("replicaCount")), ExperimentalFeatures: pulumi.Bool(conf.GetBool("experimentalFeatures")), +// Provide default values +debug := conf.GetBool("debug", false) +replicaCount := conf.GetInt("replicaCount", 1) +experimentalFeatures := conf.GetBool("experimentalFeatures", false) +Debug: pulumi.Bool(debug), +ReplicaCount: pulumi.Int(replicaCount), +ExperimentalFeatures: pulumi.Bool(experimentalFeatures),deployments/pulumi/main_test.go (1)
52-56: Handle errors during cleanup gracefullyIn the test cleanup function, capturing and logging errors from
Destroyprovides better diagnostics in case of failures during resource teardown.t.Cleanup(func() { t.Log("Destroy stack") _, err := stack.Destroy(ctx, optdestroy.Remove(), optdestroy.ProgressStreams(os.Stdout), optdestroy.ErrorProgressStreams(os.Stderr)) - require.NoError(t, err) + if err != nil { + t.Errorf("Failed to destroy stack: %v", err) + } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
deployments/pulumi/main.go(2 hunks)deployments/pulumi/main_test.go(1 hunks)deployments/pulumi/pkg/component.go(6 hunks)
🔇 Additional comments (8)
deployments/pulumi/pkg/component.go (3)
18-24: Good introduction of UpgradeMode type and associated constants
Defining UpgradeMode and its constants enhances code clarity and type safety when handling upgrade modes.
88-88: Adding Upgrade field to ComponentArgs for unified upgrade handling
The addition of Upgrade pulumix.Input[UpgradeMode] in ComponentArgs consolidates upgrade settings, simplifying the interface and reducing complexity.
375-378: Correctly setting AUTO_UPGRADE environment variable
Setting AUTO_UPGRADE to "true" when upgradeMode is UpgradeModeInApp is appropriate. Ensure no unintended side effects occur for other modes.
deployments/pulumi/main.go (1)
9-9: Added necessary import for pulumix
Including "github.com/pulumi/pulumi/sdk/v3/go/pulumix" allows the use of pulumix functions and types used in the code.
deployments/pulumi/main_test.go (4)
22-25: Structuring test cases enhances readability and scalability
Defining a testCase struct and iterating over test cases improves test maintenance and allows for easy addition of new scenarios.
26-40: Comprehensive test cases for upgrade modes
Including both nominal and upgrade scenarios ensures that the new upgrade-mode functionality is thoroughly tested.
63-73: Passing test-specific configurations correctly
The dynamic assignment of tc.config ensures each test uses its specific configuration, leading to accurate and isolated test results.
45-46: Verify isolation between test cases
Ensure that each test case operates in isolation to prevent resource conflicts, especially when using shared resources like stack names.
Consider appending the test case name to stackName to guarantee uniqueness:
stackName := "ledger-tests-pulumi-" + uuid.NewString()[:8]
+stackName += "-" + tc.nameRun the following script to check for potential conflicts:
| upgradeMode := UpgradeModeInApp | ||
| if args.Upgrade != nil { | ||
| var ( | ||
| upgradeModeChan = make(chan UpgradeMode, 1) | ||
| ) | ||
| pulumix.ApplyErr(args.Upgrade, func(upgradeMode UpgradeMode) (any, error) { | ||
| upgradeModeChan <- upgradeMode | ||
| close(upgradeModeChan) | ||
| return nil, nil | ||
| }) | ||
|
|
||
| select { | ||
| case <-ctx.Context().Done(): | ||
| return nil, ctx.Context().Err() | ||
| case upgradeMode = <-upgradeModeChan: | ||
| if upgradeMode == "" { | ||
| upgradeMode = UpgradeModeInApp | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential deadlock risk in upgradeMode initialization
The use of an unbuffered channel with pulumix.ApplyErr may lead to a deadlock if the ApplyErr callback is not invoked. This can occur if the input is not ready or an error occurs upstream.
Refactor the code to avoid using channels and handle the input more safely. Consider using ApplyT and Await to retrieve the value synchronously within the context:
upgradeMode := UpgradeModeInApp
if args.Upgrade != nil {
- var (
- upgradeModeChan = make(chan UpgradeMode, 1)
- )
- pulumix.ApplyErr(args.Upgrade, func(upgradeMode UpgradeMode) (any, error) {
- upgradeModeChan <- upgradeMode
- close(upgradeModeChan)
- return nil, nil
- })
-
- select {
- case <-ctx.Context().Done():
- return nil, ctx.Context().Err()
- case upgradeMode = <-upgradeModeChan:
- if upgradeMode == "" {
- upgradeMode = UpgradeModeInApp
- }
- }
+ upgradeModeOutput := args.Upgrade.ToOutput(ctx.Context())
+ err := upgradeModeOutput.ApplyT(func(mode UpgradeMode) (UpgradeMode, error) {
+ if mode == "" {
+ return UpgradeModeInApp, nil
+ }
+ upgradeMode = mode
+ return mode, nil
+ }).(pulumi.Output).Await(ctx.Context())
+ if err != nil {
+ return nil, err
+ }
}📝 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.
| upgradeMode := UpgradeModeInApp | |
| if args.Upgrade != nil { | |
| var ( | |
| upgradeModeChan = make(chan UpgradeMode, 1) | |
| ) | |
| pulumix.ApplyErr(args.Upgrade, func(upgradeMode UpgradeMode) (any, error) { | |
| upgradeModeChan <- upgradeMode | |
| close(upgradeModeChan) | |
| return nil, nil | |
| }) | |
| select { | |
| case <-ctx.Context().Done(): | |
| return nil, ctx.Context().Err() | |
| case upgradeMode = <-upgradeModeChan: | |
| if upgradeMode == "" { | |
| upgradeMode = UpgradeModeInApp | |
| } | |
| } | |
| upgradeMode := UpgradeModeInApp | |
| if args.Upgrade != nil { | |
| upgradeModeOutput := args.Upgrade.ToOutput(ctx.Context()) | |
| err := upgradeModeOutput.ApplyT(func(mode UpgradeMode) (UpgradeMode, error) { | |
| if mode == "" { | |
| return UpgradeModeInApp, nil | |
| } | |
| upgradeMode = mode | |
| return mode, nil | |
| }).(pulumi.Output).Await(ctx.Context()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
| Debug: pulumi.Bool(conf.GetBool("debug")), | ||
| ReplicaCount: pulumi.Int(conf.GetInt("replicaCount")), | ||
| ExperimentalFeatures: pulumi.Bool(conf.GetBool("experimentalFeatures")), | ||
| Upgrade: pulumix.Val(pulumi_ledger.UpgradeMode(config.Get(ctx, "upgrade-mode"))), |
There was a problem hiding this comment.
Ensure upgrade-mode configuration is correctly handled
Passing the upgrade-mode configuration directly may result in an empty UpgradeMode if not set. Provide a default value to prevent unexpected behavior.
Modify the code to handle a default upgrade-mode:
Upgrade: pulumix.Val(pulumi_ledger.UpgradeMode(config.Get(ctx, "upgrade-mode"))),
+// Set default upgrade mode if not provided
+upgradeMode := config.Get(ctx, "upgrade-mode")
+if upgradeMode == "" {
+ upgradeMode = "in-app" // or another appropriate default
+}
+Upgrade: pulumix.Val(pulumi_ledger.UpgradeMode(upgradeMode)),Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tools/generator/cmd/root.go (1)
57-57: Enhance the help message with time unit examplesThe help message could be more helpful by including examples of valid time units.
-rootCmd.Flags().Duration(httpClientTimeoutFlag, 0, "HTTP client timeout (default: no timeout)") +rootCmd.Flags().Duration(httpClientTimeoutFlag, 0, "HTTP client timeout (e.g., 30s, 1m, 2h) (default: no timeout)")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tools/generator/cmd/root.go(4 hunks)
🔇 Additional comments (3)
tools/generator/cmd/root.go (3)
Line range hint 22-34: LGTM: Constant definition follows established patterns
The new httpClientTimeoutFlag constant follows the existing naming conventions and is appropriately placed within the const block.
114-117: LGTM: Error handling is consistent and clear
The timeout flag retrieval and error handling follow the established patterns in the codebase.
Line range hint 119-127: Verify timeout behavior with OAuth2 client wrapper
The timeout is correctly configured for the base HTTP client. However, we should verify that the timeout setting is preserved when the client is wrapped with OAuth2 credentials.
✅ Verification successful
Let me gather more information about how the OAuth2 client is being used and configured.
Let me try one final verification to understand how the OAuth2 configuration is structured.
Based on the gathered information, I can now provide the final response:
OAuth2 client timeout configuration is correctly preserved
The OAuth2 configuration is properly handled as the HTTP client with timeout is passed directly to the OAuth2 context using context.WithValue(ctx, oauth2.HTTPClient, httpClient), and then the same client instance is used with ledgerclient.WithClient(httpClient). This approach ensures that the timeout settings are preserved when the client is wrapped with OAuth2 credentials.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any OAuth2 client timeout handling in the codebase
# to ensure consistent behavior across authentication scenarios
# Look for OAuth2 client timeout handling patterns
rg -A 5 "oauth2.*Client|http\.Client.*Timeout" --type go
# Look for any existing timeout-related tests
ast-grep --pattern 'func Test$_(.*) {
$$$
oauth2.$$$
$$$
timeout$$$
$$$
}'
Length of output: 1234
Script:
#!/bin/bash
# Search for OAuth2 client configuration and usage
ast-grep --pattern 'oauth2.Config {
$$$
}'
# Look for the actual OAuth2 client creation/wrapping
rg -A 10 "oauth2.*Config.*{|oauth2.*Token" --type go
# Check how httpClient is used with OAuth2
rg -B 5 -A 5 "WithClient.*httpClient" --type go
Length of output: 1790
Script:
#!/bin/bash
# Search for OAuth2 configuration and client wrapping
rg -A 10 'oauth2\.Config' --type go
# Look for token source configuration
rg -A 5 'TokenSource|oauth2\.Token' --type go
# Check the ledgerclient implementation
rg -A 10 'func New\(' --type go
Length of output: 5147
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
pkg/client/docs/models/operations/v2countaccountsrequest.md (1)
10-10: Enhance RequestBody field documentationThe RequestBody field lacks proper documentation. Please provide:
- A clear description of its purpose
- Expected structure and format
- A concrete example of valid request body content
internal/storage/module.go (1)
32-47: Consider adding timeout for upgrade process.While the upgrade process maintains context cancellation, consider adding a timeout to prevent indefinite upgrades.
upgradeContext, cancelContext = context.WithCancel(context.WithoutCancel(ctx)) +upgradeContext, cancelContext = context.WithTimeout(upgradeContext, 30*time.Minute)internal/storage/system/store.go (1)
38-40: Consider adding error contextWhile the implementation is correct, consider wrapping the error with additional context.
func (d *DefaultStore) IsUpToDate(ctx context.Context) (bool, error) { - return d.GetMigrator().IsUpToDate(ctx) + upToDate, err := d.GetMigrator().IsUpToDate(ctx) + if err != nil { + return false, fmt.Errorf("checking migration status: %w", err) + } + return upToDate, nil }test/migrations/upgrade_test.go (1)
76-76: LGTM: Migration call updated consistentlyThe removal of the channel parameter from
UpgradeAllBucketsis consistent with the broader API changes.Consider adding test cases to verify the behavior when migrations are already up to date using the new
IsUpToDatemethod.require.NoError(t, driver.UpgradeAllBuckets(ctx)) + +// Verify migrations are up to date +upToDate, err := driver.IsUpToDate(ctx) +require.NoError(t, err) +require.True(t, upToDate, "migrations should be up to date after upgrade")internal/storage/driver/driver.go (2)
Line range hint
165-219: Consider enhancing error handling in UpgradeAllBucketsWhile the changes simplify the interface, the error handling could be improved. Currently, if any bucket fails to migrate after retries, the method still returns nil. Consider collecting and aggregating errors from all bucket migrations.
func (d *Driver) UpgradeAllBuckets(ctx context.Context) error { + var errs []error + errCh := make(chan error, 1) buckets, err := d.systemStore.GetDistinctBuckets(ctx) if err != nil { return fmt.Errorf("getting distinct buckets: %w", err) } wp := pond.New(d.parallelBucketMigrations, len(buckets), pond.Context(ctx)) + wp.OnCompletion(func(e error) { + if e != nil { + errCh <- e + } + }) // ... rest of the function ... wp.StopAndWait() + close(errCh) + + for err := range errCh { + errs = append(errs, err) + } + + if len(errs) > 0 { + return fmt.Errorf("failed to upgrade some buckets: %v", errs) + } return nil }
221-245: LGTM: Well-implemented version check with room for optimizationThe IsUpToDate implementation is thorough and handles errors properly. However, for large systems with many buckets, consider parallelizing the bucket version checks for better performance.
func (d *Driver) IsUpToDate(ctx context.Context) (bool, error) { isUpToDate, err := d.systemStore.IsUpToDate(ctx) if err != nil { return false, fmt.Errorf("checking if system store is up to date: %w", err) } if !isUpToDate { return false, nil } buckets, err := d.systemStore.GetDistinctBuckets(ctx) if err != nil { return false, fmt.Errorf("getting distinct buckets: %w", err) } + type result struct { + bucket string + isUpToDate bool + err error + } + results := make(chan result, len(buckets)) + + for _, b := range buckets { + go func(bucket string) { + isUpToDate, err := d.bucketFactory.Create(bucket).IsUpToDate(ctx) + results <- result{bucket, isUpToDate, err} + }(b) + } + + for range buckets { + r := <-results + if r.err != nil { + return false, fmt.Errorf("checking if bucket '%s' is up to date: %w", r.bucket, r.err) + } + if !r.isUpToDate { + return false, nil + } + } return true, nil }internal/api/v2/controllers_ledgers_create.go (1)
Line range hint
38-47: Consider refactoring error handling for better maintainability.The current switch-case structure with multiple
errors.Ischecks could be refactored for better readability and maintainability.Consider using a map-based approach or separate error handling functions:
- switch { - case errors.Is(err, system.ErrInvalidLedgerConfiguration{}) || - errors.Is(err, ledger.ErrInvalidLedgerName{}) || - errors.Is(err, ledger.ErrInvalidBucketName{}): - api.BadRequest(w, common.ErrValidation, err) - case errors.Is(err, system.ErrBucketOutdated): - api.BadRequest(w, common.ErrOutdatedSchema, err) - case errors.Is(err, system.ErrLedgerAlreadyExists): - api.BadRequest(w, common.ErrLedgerAlreadyExists, err) - default: - common.HandleCommonErrors(w, r, err) + if err := handleLedgerCreationError(w, r, err); err != nil { + common.HandleCommonErrors(w, r, err) + }Add a new function:
func handleLedgerCreationError(w http.ResponseWriter, r *http.Request, err error) error { errorMappings := map[error]struct { apiErr error fn func(http.ResponseWriter, error, error) }{ system.ErrInvalidLedgerConfiguration{}: {common.ErrValidation, api.BadRequest}, ledger.ErrInvalidLedgerName{}: {common.ErrValidation, api.BadRequest}, ledger.ErrInvalidBucketName{}: {common.ErrValidation, api.BadRequest}, system.ErrBucketOutdated: {common.ErrOutdatedSchema, api.BadRequest}, system.ErrLedgerAlreadyExists: {common.ErrLedgerAlreadyExists, api.BadRequest}, } for errType, handler := range errorMappings { if errors.Is(err, errType) { handler.fn(w, handler.apiErr, err) return nil } } return err }pkg/client/docs/models/components/v2bulkresponse.md (1)
11-11: Format the bare URL in the example fieldThe example URL should be properly formatted in markdown to prevent potential rendering issues.
-| `Details` | **string* | :heavy_minus_sign: | N/A | https://play.numscript.org/?payload=eyJlcnJvciI6ImFjY291bnQgaGFkIGluc3VmZmljaWVudCBmdW5kcyJ9 | +| `Details` | **string* | :heavy_minus_sign: | N/A | `https://play.numscript.org/?payload=eyJlcnJvciI6ImFjY291bnQgaGFkIGluc3VmZmljaWVudCBmdW5kcyJ9` |🧰 Tools
🪛 Markdownlint (0.35.0)
11-11: null
Bare URL used(MD034, no-bare-urls)
pkg/generate/generator.go (1)
152-158: Enhance error handling with more contextThe error handling for HTTP 400 responses could be improved by including more context about the failed request.
Consider this enhancement:
- if response.HTTPMeta.Response.StatusCode == http.StatusBadRequest { - return nil, fmt.Errorf( - "unexpected error: %s [%s]", - response.V2BulkResponse.ErrorMessage, - response.V2BulkResponse.ErrorCode, - ) - } + if response.HTTPMeta.Response.StatusCode == http.StatusBadRequest { + return nil, fmt.Errorf( + "bulk operation failed with status %d: %s [%s]", + response.HTTPMeta.Response.StatusCode, + response.V2BulkResponse.ErrorMessage, + response.V2BulkResponse.ErrorCode, + ) + }docs/api/README.md (1)
3804-3815: Fix duplicate heading in documentationThe static analysis tool detected a duplicate heading in the schema definition.
Consider renaming one of the duplicate headings to be more specific about its content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yaml
📒 Files selected for processing (34)
cmd/buckets_upgrade.go(1 hunks)cmd/root.go(1 hunks)docs/api/README.md(3 hunks)internal/api/common/errors.go(0 hunks)internal/api/v2/controllers_ledgers_create.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/default_bucket_test.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/buckets_generated_test.go(2 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(6 hunks)internal/storage/driver/system_generated_test.go(1 hunks)internal/storage/ledger/legacy/main_test.go(1 hunks)internal/storage/module.go(3 hunks)internal/storage/system/store.go(2 hunks)pkg/client/docs/models/components/v2bulkresponse.md(1 hunks)pkg/client/docs/models/components/v2errorsenum.md(1 hunks)pkg/client/docs/models/operations/v2countaccountsrequest.md(1 hunks)pkg/client/docs/models/operations/v2counttransactionsrequest.md(1 hunks)pkg/client/docs/models/operations/v2listaccountsrequest.md(0 hunks)pkg/client/docs/models/operations/v2listtransactionsrequest.md(0 hunks)pkg/client/docs/sdks/v2/README.md(0 hunks)pkg/client/formance.go(1 hunks)pkg/client/models/components/v2bulkresponse.go(2 hunks)pkg/client/models/components/v2errorsenum.go(2 hunks)pkg/client/models/operations/v2countaccounts.go(1 hunks)pkg/client/models/operations/v2counttransactions.go(1 hunks)pkg/client/models/operations/v2listaccounts.go(1 hunks)pkg/client/models/operations/v2listtransactions.go(0 hunks)pkg/generate/generator.go(2 hunks)test/migrations/upgrade_test.go(1 hunks)tools/generator/cmd/root.go(6 hunks)
💤 Files with no reviewable changes (5)
- internal/api/common/errors.go
- pkg/client/docs/models/operations/v2listaccountsrequest.md
- pkg/client/models/operations/v2listtransactions.go
- pkg/client/docs/sdks/v2/README.md
- pkg/client/docs/models/operations/v2listtransactionsrequest.md
✅ Files skipped from review due to trivial changes (1)
- pkg/client/formance.go
🧰 Additional context used
🪛 Markdownlint (0.35.0)
pkg/client/docs/models/components/v2bulkresponse.md
11-11: null
Bare URL used
(MD034, no-bare-urls)
8-8: Missing link or image reference definition: "components.v2bulkelementresult"
Reference links and images should use a label that is defined
(MD052, reference-links-images)
docs/api/README.md
3802-3802: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (35)
pkg/client/docs/models/operations/v2countaccountsrequest.md (1)
6-10: Verify if these documentation changes are related to the PR objective
The removal of the Query field and table formatting changes seem unrelated to the PR's objective of "allowing multiple migration mode on pulumi program". Please clarify if these changes should be in a separate PR focused on API documentation updates.
✅ Verification successful
Based on the git diff results from previous runs, I can now provide a final response:
Documentation changes are part of a broader API update
The changes are part of a larger set of API-related updates in this PR, which includes:
- Removal of the
Queryfield from multiple API endpoints (CountAccounts, ListAccounts, CountTransactions, ListTransactions) - Updates to error handling with new error codes and response structures
- Changes to the OpenAPI specifications
- Corresponding updates in the generated client code and documentation
These changes appear to be intentionally bundled together as they represent a cohesive update to the API structure, even though the PR title only mentions Pulumi-related changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are related changes in pulumi-related files
rg -l "Query" "deployments/pulumi/"
# Check for any connections between these models and pulumi code
rg -l "CountAccounts|CountTransactions" "deployments/pulumi/"
Length of output: 95
Script:
#!/bin/bash
# Let's check the git history for this specific file to understand the context of changes
git log -p -- pkg/client/docs/models/operations/v2countaccountsrequest.md
# Let's also check for any related changes in the PR
git diff --name-only HEAD~1
# Check if there are any other documentation changes in this PR
fd -e md -x git diff HEAD~1 -- {}
Length of output: 33499
Script:
#!/bin/bash
# Let's check the OpenAPI changes to understand if this is part of a broader API change
git diff HEAD~1 openapi.yaml openapi/v2.yaml | grep -A 10 -B 10 "Query"
# Let's also check if there are any related changes in the API implementation
rg -l "Query.*string.*match" internal/api/
Length of output: 116
pkg/client/docs/models/operations/v2counttransactionsrequest.md (1)
10-10: Same documentation improvements needed as V2CountAccountsRequest
The RequestBody field has the same documentation gaps as identified in the V2CountAccountsRequest file. Please apply the same documentation improvements here.
internal/storage/bucket/default_bucket_test.go (1)
34-34: LGTM: Migration call simplified
The removal of the channel parameter aligns with the broader refactoring of the migration interface, making the test more straightforward while maintaining its core validation purpose.
internal/storage/bucket/migrations.go (2)
Line range hint 27-39: LGTM: Migration logic simplified while maintaining core functionality
The migration implementation has been streamlined while preserving essential functionality:
- Error handling focuses on the critical
ErrAlreadyUpToDatecase - Tracing support remains intact for observability
- The migration loop is clear and focused
27-27: Verify version tracking requirements
The removal of minimalVersionReached channel simplifies the migration logic, but we should verify that version tracking requirements are met elsewhere if needed.
✅ Verification successful
Let me gather more information about the version tracking implementation by checking the migration logic in the default bucket.
Let me try a different approach to check the migration implementation.
Version tracking is properly maintained despite channel removal
The version tracking functionality is still robustly implemented in the codebase:
MinimalSchemaVersionconstant is defined and used inDefaultBucket- Version checks are performed through
IsMinimalVersionReachedmethod - Migration process maintains version tracking via
GetLastVersionandIsUpToDatechecks - The removal of the channel parameter simplifies the code while preserving the core version validation logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version tracking logic in other files
rg -A 5 "MinimalSchemaVersion|SchemaVersion"
Length of output: 1222
Script:
#!/bin/bash
# Check the implementation of version tracking in DefaultBucket
ast-grep --pattern 'func (b *DefaultBucket) IsMinimalVersionReached(ctx context.Context) (bool, error) {
$$$
}'
# Also check the migrator implementation
ast-grep --pattern 'func GetMigrator(db *bun.DB, name string) *migrations.Migrator {
$$$
}'
Length of output: 211
Script:
#!/bin/bash
# Check the implementation details of DefaultBucket
rg -A 10 "func \(b \*DefaultBucket\) IsMinimalVersionReached"
# Look for migration-related functions in the bucket package
rg -A 5 "GetMigrator|UpByOne|GetLastVersion" internal/storage/bucket/
Length of output: 4560
cmd/root.go (1)
47-47: LGTM: Consistent interface simplification
The removal of the channel parameter from UpgradeAllBuckets maintains consistency with the broader migration interface changes while preserving proper database connection handling.
internal/storage/bucket/bucket.go (1)
13-13: LGTM! Verify implementations of the updated interface.
The simplified Migrate method signature improves maintainability. However, we should ensure all implementations have been updated correctly.
✅ Verification successful
Let me try a different approach to find the implementations using rg to search for the Migrate method.
Let me verify if there are any other implementations of the Migrate method that might be using the old signature with the channel parameter.
All implementations have been updated to match the new interface signature
Based on the search results, all implementations of the Migrate method have been properly updated to use the new signature Migrate(ctx context.Context, options ...migrations.Option) error. The implementations were found in:
internal/storage/bucket/default_bucket.gointernal/storage/system/store.gointernal/storage/driver/buckets_generated_test.go(mock)internal/storage/driver/system_generated_test.go(mock)
No implementations using the old signature with the channel parameter were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of the Bucket interface to verify they've been updated
# Look for struct types that implement Migrate method
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ *$_) Migrate(ctx context.Context, $$$) error {
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Search for Migrate method implementations
rg "func.*Migrate.*context\.Context.*\) error" -A 5
Length of output: 2489
Script:
#!/bin/bash
# Search for any Migrate implementations that might use channels
rg "func.*Migrate.*chan" -A 5
Length of output: 31
internal/storage/module.go (1)
15-26: LGTM! Well-implemented health check.
The health check implementation is robust:
- Properly handles context
- Clear error messages
- Appropriate error propagation
cmd/buckets_upgrade.go (1)
33-33: LGTM! Consistent with interface changes.
The simplified UpgradeAllBuckets call aligns with the interface changes while maintaining the existing functionality.
internal/storage/ledger/legacy/main_test.go (1)
72-72: Verify migration call signature consistency across test files
The removal of the channel parameter from b.Migrate(ctx) aligns with the broader API changes. Let's verify consistency across other test files.
✅ Verification successful
Migration call signature is consistent across the codebase
The verification shows that all Migrate function calls across test files follow the same pattern, using only the context parameter (or context and db parameters for system migrations). No instances of channel parameters were found, confirming that the changes are consistent throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Migrate calls with channel parameters in test files
rg -g "**/*_test.go" "Migrate\(.*chan\s+struct{}" .
Length of output: 187
Script:
#!/bin/bash
# Search for Migrate function calls in test files
rg -g "*_test.go" "\.Migrate\(" -A 1
Length of output: 1335
internal/storage/system/store.go (1)
27-27: LGTM: Good addition to the Store interface
The IsUpToDate method enhances the migration API by providing a way to check migration status.
internal/storage/driver/buckets_generated_test.go (1)
Line range hint 116-132: LGTM: Generated mock code is correct.
The changes to the Migrate method signature are properly generated and align with the removal of the minimalVersionReached channel parameter.
internal/storage/driver/system_generated_test.go (1)
119-132: LGTM: Generated mock code is correct.
The new IsUpToDate method and its mock recorder are properly generated following gomock's patterns.
tools/generator/cmd/root.go (5)
33-34: LGTM: Well-defined constants.
The new constants for HTTP client timeout and debug flags are properly defined and follow the existing naming convention.
58-59: LGTM: Clear flag definitions with good documentation.
The flags are well-documented with clear default values and descriptions.
126-127: Improved logging context and messages.
Good improvements in logging:
- Using a logger context instead of command context
- Added informative messages for ledger creation and generation progress
Also applies to: 170-170, 195-195
116-124: LGTM: Consistent error handling.
The error handling for the new flags follows the same pattern as other flags in the file, maintaining consistency.
Line range hint 129-139: Verify HTTP client configuration impact.
The HTTP client configuration includes both timeout and connection pooling settings. While the implementation is correct, ensure that:
- The connection pool size (vus) is appropriate for your use case
- The timeout setting doesn't conflict with long-running operations
✅ Verification successful
Based on the gathered information, I can now provide a final response:
HTTP client configuration is appropriate for the use case
The HTTP client configuration is well-designed for its purpose:
- The timeout is configurable via CLI flag with a safe default (no timeout)
- The connection pool size matches the number of virtual users (vus), which is optimal for concurrent operations
- No long-running operations or streaming endpoints were found in the generator tool that would conflict with timeouts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential timeout-sensitive operations
rg -A 5 "client\." | grep -E "stream|watch|long|poll"
Length of output: 55
Script:
#!/bin/bash
# Let's check how the HTTP client is being used in the codebase
rg -A 5 "httpClient\."
# Also check for any request-related configurations or timeouts
rg -B 2 -A 2 "Timeout|timeout"
Length of output: 42794
Script:
#!/bin/bash
# Let's check how the HTTP client is being used in the code
rg -A 5 "httpClient\." tools/generator/cmd/
# Also check for any long-running operations in the generator
rg -B 2 -A 2 "for|while|loop" tools/generator/cmd/
Length of output: 1699
internal/storage/driver/driver_test.go (3)
54-55: LGTM: Test correctly updated for new Migrate signature
The test case has been properly updated to match the simplified Migrate method signature.
94-95: LGTM: Concurrent migration test properly updated
The concurrent bucket test case correctly implements the new Migrate signature while maintaining the original test coverage.
141-143: LGTM: Error handling test properly maintained
The error handling test case has been correctly updated while preserving the important error simulation logic.
Also applies to: 151-153
internal/storage/bucket/default_bucket.go (1)
41-42: LGTM: Clean signature update
The Migrate method has been correctly simplified while maintaining its core functionality.
pkg/client/docs/models/components/v2errorsenum.md (1)
24-25: LGTM! New error enum value is well documented.
The addition of V2ErrorsEnumOutdatedSchema follows the established naming convention and maintains consistent documentation format.
pkg/client/models/components/v2bulkresponse.go (2)
6-9: LGTM! Well-structured error handling fields.
The new fields provide comprehensive error reporting capabilities with:
- Required fields: ErrorCode (using V2ErrorsEnum) and ErrorMessage
- Optional Details field with proper omitempty tag
19-38: LGTM! Robust getter methods implementation.
The getter methods are well-implemented with:
- Consistent nil checks
- Appropriate zero values/nil returns
- Pattern matching existing GetData method
internal/api/v2/controllers_ledgers_create.go (1)
41-41: LGTM! Error handling updated correctly.
The change from ErrBucketOutdated to ErrOutdatedSchema aligns with the new error enum structure.
pkg/client/models/operations/v2countaccounts.go (1)
13-14: Verify the impact of Query field removal
This is a generated file, and the removal of the Query field represents a significant API change that may affect existing clients. Please ensure:
- The API specification (OpenAPI/Swagger) has been updated accordingly
- This change is documented in the migration guide
- Clients are notified about this breaking change
✅ Verification successful
Based on the search results, I can now generate the final response:
The Query field removal appears to be part of API evolution
Looking at the codebase, the removal of the Query field from v2countaccounts appears to be consistent with the API design pattern used across other v2 endpoints. The remaining query parameters (Ledger and Pit) align with similar endpoints like v2counttransactions, which also use a RequestBody for complex query criteria instead of query parameters.
This change appears to be a deliberate API design choice where:
- Simple filters are handled via query parameters (Pit)
- Complex query criteria are moved to the RequestBody
- This pattern is consistent across v2 endpoints in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Query field usages in the codebase
rg -l "Query.*queryParam.*query" --type go
Length of output: 44
Script:
#!/bin/bash
# Let's try a broader search for Query-related usages and references
rg -i "query.*param" pkg/client/models/operations/ -A 2
# Also search for any references to v2countaccounts specifically
rg "v2countaccounts" --type go -A 3
# Check git history for the specific file to understand the change
git log -p -- pkg/client/models/operations/v2countaccounts.go | head -n 50
Length of output: 21379
pkg/client/models/operations/v2counttransactions.go (1)
13-14: Consistent with v2countaccounts.go changes
The removal of the Query field here matches the changes in v2countaccounts.go, maintaining API consistency. However, as this is also a generated file, ensure that the underlying API specification changes are properly reviewed.
pkg/client/models/components/v2errorsenum.go (2)
30-30: Addition of OUTDATED_SCHEMA aligns with migration mode feature
The new error enum supports the multiple migration mode feature by providing proper error handling for schema version mismatches. This is a good addition that will help identify migration-related issues.
75-76: Verify error handling implementation
The UnmarshalJSON implementation correctly handles the new OUTDATED_SCHEMA enum. Please ensure that the error handling is properly implemented in the migration logic.
✅ Verification successful
Error handling for OUTDATED_SCHEMA is properly implemented
The error handling for OUTDATED_SCHEMA is correctly implemented across the codebase:
- In
middleware_resolver.go, it checks if the database schema is up-to-date and returns a proper error response withErrOutdatedSchema - The error handling is well-tested in
middleware_resolver_test.gowith specific test cases for outdated schema scenarios - The error code is consistently used in ledger creation (
controllers_ledgers_create_test.go) when handling outdated bucket errors - The enum is properly integrated into the error types system (
v2errorsenum.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for OUTDATED_SCHEMA usage in error handling
rg "OUTDATED_SCHEMA" --type go -B 2 -A 2
# Look for related error handling patterns
rg "ErrOutdatedSchema|handleSchemaError" --type go
Length of output: 1661
Script:
#!/bin/bash
# Let's check the error handling implementation in middleware_resolver.go
cat internal/api/common/middleware_resolver.go
# Also check the test cases
cat internal/api/common/middleware_resolver_test.go
cat internal/api/v2/controllers_ledgers_create_test.go
Length of output: 8962
pkg/client/models/operations/v2listaccounts.go (1)
22-24: LGTM - Auto-generated file
The changes appear to be formatting adjustments in the auto-generated code. No functional changes observed.
pkg/generate/generator.go (2)
146-146: LGTM: Atomic parameter addition
The addition of the atomic parameter with a pointer to true ensures atomic execution of bulk operations.
160-168: LGTM: Bulk response error handling
The error handling for individual bulk elements is well implemented, checking for error type and providing detailed error information.
docs/api/README.md (2)
600-603: LGTM: Error response fields documented
The documentation correctly reflects the new error response structure with errorCode, errorMessage, and details fields.
3250-3250: LGTM: New error code documented
The OUTDATED_SCHEMA error code is properly documented in the V2ErrorsEnum.
| @@ -84,7 +84,7 @@ func TestLedgersCreate(t *testing.T) { | |||
| expectedBackendCall: true, | |||
| returnErr: system.ErrBucketOutdated, | |||
| expectStatusCode: http.StatusBadRequest, | |||
| expectErrorCode: common.ErrBucketOutdated, | |||
| expectErrorCode: common.ErrOutdatedSchema, | |||
There was a problem hiding this comment.
💡 Codebase verification
Error code inconsistency detected - ErrBucketOutdated still present in codebase
The change to ErrOutdatedSchema is incomplete as ErrBucketOutdated is still being used in multiple locations:
internal/storage/driver/driver.go: Returnssystemcontroller.ErrBucketOutdatedinternal/api/v2/controllers_ledgers_create.go: Checks forsystem.ErrBucketOutdatedinternal/controller/system/errors.go: DefinesErrBucketOutdated
This creates an inconsistency where the same error condition is represented by two different error codes across the codebase.
🔗 Analysis chain
LGTM - Error code update aligns with new schema
The change from ErrBucketOutdated to ErrOutdatedSchema correctly reflects the updated error categorization in the main code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of ErrOutdatedSchema across the codebase
# Check for any remaining references to ErrBucketOutdated
rg "ErrBucketOutdated"
# Check for consistent usage of ErrOutdatedSchema
rg "ErrOutdatedSchema"
Length of output: 1011
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
from
0391075 to
9001f39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
pkg/client/docs/models/operations/v2countaccountsrequest.md (1)
9-10: Consider adding descriptions and examples for Pit and RequestBody fields.The
PitandRequestBodyfields currently show "N/A" for their descriptions. To improve API usability:
- Add a description for
Pitexplaining its purpose (e.g., point-in-time query parameter)- Add a description and example for
RequestBodyshowing the expected payload structure| `Pit` | [*time.Time](https://pkg.go.dev/time#Time) | :heavy_minus_sign: | N/A | | -| `RequestBody` | map[string]*any* | :heavy_minus_sign: | N/A | | +| `RequestBody` | map[string]*any* | :heavy_minus_sign: | Filter conditions for counting accounts | {"address": "user:001", "metadata.type": "checking"} |pkg/client/docs/models/components/v2bulkresponse.md (1)
9-11: Consider escaping the example URL in Details fieldThe example URL in the Details field should be properly escaped in the markdown table to prevent potential rendering issues.
-| `Details` | **string* | :heavy_minus_sign: | N/A | https://play.numscript.org/?payload=eyJlcnJvciI6ImFjY291bnQgaGFkIGluc3VmZmljaWVudCBmdW5kcyJ9 | +| `Details` | **string* | :heavy_minus_sign: | N/A | `https://play.numscript.org/?payload=eyJlcnJvciI6ImFjY291bnQgaGFkIGluc3VmZmljaWVudCBmdW5kcyJ9` |🧰 Tools
🪛 Markdownlint (0.35.0)
11-11: null
Bare URL used(MD034, no-bare-urls)
pkg/generate/generator.go (1)
146-146: Consider documenting the atomic flag behaviorThe atomic flag is set to true, which ensures bulk operations are treated as a single unit. Consider adding a comment explaining this behavior and its implications.
- Atomic: pointer.For(true), + // Ensure bulk operations are atomic - all operations succeed or all fail + Atomic: pointer.For(true),tools/generator/cmd/root.go (1)
126-127: Consider adding timeout validation and logging.While the implementation is correct, consider adding:
- Validation for extremely large timeout values
- Debug logging for the configured timeout value
logger := logging.NewDefaultLogger(cmd.OutOrStdout(), debug, false, false) ctx := logging.ContextWithLogger(cmd.Context(), logger) + +if httpClientTimeout > 24*time.Hour { + logger.Warnf("HTTP client timeout is set to a very large value: %v", httpClientTimeout) +} +logger.Debugf("Configuring HTTP client with timeout: %v", httpClientTimeout) httpClient := &http.Client{ Timeout: httpClientTimeout,Also applies to: 130-130
internal/storage/driver/driver.go (2)
Line range hint
165-219: Consider adding metrics for migration progress.The parallel bucket migration implementation is solid, but monitoring could be improved.
Consider adding metrics to track:
- Number of buckets being upgraded
- Migration duration
- Retry attempts
func (d *Driver) UpgradeAllBuckets(ctx context.Context) error { + start := time.Now() + defer func() { + d.meter.RecordBatch(ctx, + []metric.Measurement{ + d.migrationDurationMetric.Record(time.Since(start).Seconds()), + }, + ) + }() buckets, err := d.systemStore.GetDistinctBuckets(ctx)
221-245: LGTM! Well-implemented version check with proper error handling.The new
HasReachMinimalVersionmethod:
- Correctly checks both system store and bucket versions
- Provides clear error messages
- Follows fail-fast principle
However, consider caching the result for a short duration to prevent frequent database queries.
+ type versionCache struct { + result bool + expiry time.Time + } + var cache atomic.Value // stores *versionCache + func (d *Driver) HasReachMinimalVersion(ctx context.Context) (bool, error) { + if cached, ok := cache.Load().(*versionCache); ok && time.Now().Before(cached.expiry) { + return cached.result, nil + } + isUpToDate, err := d.systemStore.IsUpToDate(ctx) // ... existing code ... + result := true + cache.Store(&versionCache{ + result: result, + expiry: time.Now().Add(5 * time.Second), + }) + return result, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lockpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yaml
📒 Files selected for processing (34)
cmd/buckets_upgrade.go(1 hunks)cmd/root.go(1 hunks)docs/api/README.md(3 hunks)internal/api/common/errors.go(0 hunks)internal/api/v2/controllers_ledgers_create.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/default_bucket_test.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/buckets_generated_test.go(2 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(6 hunks)internal/storage/driver/system_generated_test.go(1 hunks)internal/storage/ledger/legacy/main_test.go(1 hunks)internal/storage/module.go(3 hunks)internal/storage/system/store.go(2 hunks)pkg/client/docs/models/components/v2bulkresponse.md(1 hunks)pkg/client/docs/models/components/v2errorsenum.md(1 hunks)pkg/client/docs/models/operations/v2countaccountsrequest.md(1 hunks)pkg/client/docs/models/operations/v2counttransactionsrequest.md(1 hunks)pkg/client/docs/models/operations/v2listaccountsrequest.md(0 hunks)pkg/client/docs/models/operations/v2listtransactionsrequest.md(0 hunks)pkg/client/docs/sdks/v2/README.md(0 hunks)pkg/client/formance.go(1 hunks)pkg/client/models/components/v2bulkresponse.go(2 hunks)pkg/client/models/components/v2errorsenum.go(2 hunks)pkg/client/models/operations/v2countaccounts.go(1 hunks)pkg/client/models/operations/v2counttransactions.go(1 hunks)pkg/client/models/operations/v2listaccounts.go(1 hunks)pkg/client/models/operations/v2listtransactions.go(0 hunks)pkg/generate/generator.go(2 hunks)test/migrations/upgrade_test.go(1 hunks)tools/generator/cmd/root.go(6 hunks)
💤 Files with no reviewable changes (5)
- internal/api/common/errors.go
- pkg/client/docs/models/operations/v2listaccountsrequest.md
- pkg/client/models/operations/v2listtransactions.go
- pkg/client/docs/sdks/v2/README.md
- pkg/client/docs/models/operations/v2listtransactionsrequest.md
🚧 Files skipped from review as they are similar to previous changes (21)
- pkg/client/docs/models/components/v2errorsenum.md
- internal/api/v2/controllers_ledgers_create_test.go
- pkg/client/docs/models/operations/v2counttransactionsrequest.md
- internal/api/v2/controllers_ledgers_create.go
- test/migrations/upgrade_test.go
- internal/storage/bucket/default_bucket_test.go
- cmd/root.go
- cmd/buckets_upgrade.go
- internal/storage/ledger/legacy/main_test.go
- internal/storage/system/store.go
- pkg/client/models/components/v2errorsenum.go
- pkg/client/models/operations/v2counttransactions.go
- pkg/client/models/operations/v2listaccounts.go
- internal/storage/bucket/bucket.go
- pkg/client/models/operations/v2countaccounts.go
- internal/storage/bucket/default_bucket.go
- internal/storage/module.go
- pkg/client/formance.go
- internal/storage/bucket/migrations.go
- internal/storage/driver/system_generated_test.go
- internal/storage/driver/buckets_generated_test.go
🧰 Additional context used
🪛 Markdownlint (0.35.0)
pkg/client/docs/models/components/v2bulkresponse.md
11-11: null
Bare URL used
(MD034, no-bare-urls)
8-8: Missing link or image reference definition: "components.v2bulkelementresult"
Reference links and images should use a label that is defined
(MD052, reference-links-images)
docs/api/README.md
3802-3802: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (11)
pkg/client/docs/models/operations/v2countaccountsrequest.md (1)
6-10: LGTM! Well-structured fields table.
The table is well-formatted with consistent column widths and clear headers.
pkg/client/models/components/v2bulkresponse.go (2)
6-9: LGTM: Well-structured error handling fields
The new error handling fields in V2BulkResponse follow a good pattern:
ErrorCodeuses enum for standardized error codesErrorMessageprovides human-readable contextDetailsis optional and allows for extended error information
19-38: LGTM: Consistent getter methods implementation
The new getter methods follow good practices:
- Consistent nil checks
- Return appropriate zero values
- Match the pattern of existing
GetDatamethod
pkg/generate/generator.go (2)
152-158: LGTM: Improved HTTP error handling
Good addition of HTTP status code check with clear error messaging that includes both the error message and code.
160-168: LGTM: Thorough bulk response validation
Good practice to check each element in the bulk response for errors, providing detailed error information when found.
docs/api/README.md (2)
3794-3797: LGTM: Enhanced error handling in bulk response schema
The addition of error response fields (errorCode, errorMessage, details) to the V2BulkResponse schema improves the API documentation by clearly specifying how errors are communicated.
3250-3250: LGTM: Improved error code naming
The addition of OUTDATED_SCHEMA error code is a good improvement over the previous BUCKET_OUTDATED, as it more accurately describes the error condition.
tools/generator/cmd/root.go (3)
33-34: LGTM! Clear and descriptive flag names.
The new flag constants follow good naming conventions and are self-descriptive.
58-59: LGTM! Well-documented flags with appropriate default values.
The flags are properly defined with clear descriptions and sensible defaults:
- HTTP client timeout defaults to no timeout (0)
- Debug flag defaults to false
116-124: LGTM! Proper error handling for flag values.
The error handling for the new flags follows the established pattern and provides clear error messages.
internal/storage/driver/driver_test.go (1)
54-55: LGTM! Test cases properly updated for new migration signature.
The test cases have been correctly updated to reflect the removal of the minimalVersionReached channel parameter. The test coverage remains comprehensive, testing both successful and error scenarios.
Also applies to: 94-95, 141-143, 151-153
| allOf | ||
|
|
||
| |Name|Type|Required|Restrictions|Description| | ||
| |---|---|---|---|---| | ||
| |*anonymous*|object|false|none|none| | ||
| |» data|[[V2BulkElementResult](#schemav2bulkelementresult)]|false|none|none| | ||
|
|
||
| and | ||
|
|
||
| |Name|Type|Required|Restrictions|Description| | ||
| |---|---|---|---|---| | ||
| |data|[[V2BulkElementResult](#schemav2bulkelementresult)]|true|none|none| | ||
| |*anonymous*|[V2ErrorResponse](#schemav2errorresponse)|false|none|none| |
There was a problem hiding this comment.
Fix duplicate heading in schema composition
The schema composition using allOf has duplicate headings which could cause confusion in the generated documentation.
Apply this fix to resolve the duplicate heading:
-allOf
-
-|Name|Type|Required|Restrictions|Description|
-|---|---|---|---|---|
-|*anonymous*|object|false|none|none|
-|» data|[[V2BulkElementResult](#schemav2bulkelementresult)]|false|none|none|
-
-and
-
|Name|Type|Required|Restrictions|Description|
|---|---|---|---|---|
|*anonymous*|[V2ErrorResponse](#schemav2errorresponse)|false|none|none|Committable suggestion skipped: line range outside the PR's diff.
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
from
9001f39 to
57e71f1
Compare
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
3 times, most recently
from
c271689 to
82ab12c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
deployments/pulumi/pkg/component.go (2)
18-24: Add documentation for the UpgradeMode type and its constantsWhile the type and constants are well-named, adding documentation would help users understand the purpose and behavior of each upgrade mode.
Add documentation comments:
+// UpgradeMode defines the strategy for handling database migrations type UpgradeMode string const ( + // UpgradeModeDisabled indicates that no automatic upgrades should be performed UpgradeModeDisabled UpgradeMode = "disabled" + // UpgradeModeJob indicates that upgrades should be performed via a Kubernetes job UpgradeModeJob UpgradeMode = "job" + // UpgradeModeInApp indicates that upgrades should be performed within the application UpgradeModeInApp UpgradeMode = "in-app" )
Line range hint
517-553: Fix incorrect RestartPolicy for migration jobThe
RestartPolicyis set to "Always", which is not a valid value for Jobs in Kubernetes. Jobs should use either "OnFailure" or "Never" as their restart policy.Apply this fix:
-RestartPolicy: pulumi.String("Always"), +RestartPolicy: pulumi.String("OnFailure"),Reference: Kubernetes Jobs documentation
tools/generator/cmd/root.go (5)
126-127: Consider adding log level configurationWhile debug logging is now configurable, consider adding more granular log level control beyond just debug/non-debug.
- logger := logging.NewDefaultLogger(cmd.OutOrStdout(), debug, false, false) + logLevel := "info" + if debug { + logLevel = "debug" + } + logger := logging.NewDefaultLogger(cmd.OutOrStdout(), logLevel == "debug", false, false)
Line range hint
170-195: Improve error handling in ledger creationThe error handling for ledger creation could be more specific about the type of validation error encountered.
- if !errors.As(err, &sdkError) || (sdkError.ErrorCode != components.V2ErrorsEnumLedgerAlreadyExists && - sdkError.ErrorCode != components.V2ErrorsEnumValidation) { + if !errors.As(err, &sdkError) { + return fmt.Errorf("failed to create ledger: %w", err) + } + switch sdkError.ErrorCode { + case components.V2ErrorsEnumLedgerAlreadyExists: + logging.FromContext(ctx).Infof("Ledger '%s' already exists", targetedLedger) + case components.V2ErrorsEnumValidation: + logging.FromContext(ctx).Warnf("Validation error creating ledger '%s': %s", targetedLedger, sdkError.Message) + default: return fmt.Errorf("failed to create ledger: %w", err) - } + }
Line range hint
165-219: Consider adding progress tracking for bucket upgradesThe UpgradeAllBuckets implementation could benefit from progress tracking to provide better visibility into the upgrade process.
func (d *Driver) UpgradeAllBuckets(ctx context.Context) error { + type upgradeStatus struct { + bucket string + status string + error error + } + statusChan := make(chan upgradeStatus, len(buckets)) wp := pond.New(d.parallelBucketMigrations, len(buckets), pond.Context(ctx)) for _, bucketName := range buckets { + bucketName := bucketName // Capture for goroutine wp.Submit(func() { + defer func() { + statusChan <- upgradeStatus{bucket: bucketName, status: "completed"} + }() logger := logging.FromContext(ctx).WithFields(map[string]any{ "bucket": bucketName, })
Line range hint
221-245: LGTM: Well-structured version check implementationThe HasReachMinimalVersion method is well-implemented with proper error handling and clear logic flow.
However, consider adding metrics to track version status:
+ if d.meter != nil { + upToDateCounter, _ := d.meter.Int64Counter("system.version.up_to_date") + upToDateCounter.Add(ctx, 1) + }
Line range hint
235-243: Consider parallel version checks for better performanceThe bucket version checks could be parallelized for better performance with many buckets.
- for _, b := range buckets { + type versionCheck struct { + bucket string + hasMinVersion bool + err error + } + results := make(chan versionCheck, len(buckets)) + for _, b := range buckets { + b := b + go func() { + hasMinimalVersion, err := d.bucketFactory.Create(b).HasMinimalVersion(ctx) + results <- versionCheck{bucket: b, hasMinVersion: hasMinimalVersion, err: err} + }() + } + for range buckets { + result := <-results + if result.err != nil { + return false, fmt.Errorf("checking if bucket '%s' is up to date: %w", result.bucket, result.err) + } + if !result.hasMinVersion { + return false, nil + } + }internal/storage/driver/driver.go (2)
Line range hint
165-219: Improve error handling and resource management in parallel migrations.The current implementation has several concerns that should be addressed:
- Migration errors are logged but not propagated to the caller
- Context cancellation errors are silently ignored
- The error channel is recreated in each iteration unnecessarily
Consider this improved implementation:
func (d *Driver) UpgradeAllBuckets(ctx context.Context) error { buckets, err := d.systemStore.GetDistinctBuckets(ctx) if err != nil { return fmt.Errorf("getting distinct buckets: %w", err) } wp := pond.New(d.parallelBucketMigrations, len(buckets), pond.Context(ctx)) + errChan := make(chan error, 1) // Create once outside the loop + var migrationErrors []error for _, bucketName := range buckets { wp.Submit(func() { logger := logging.FromContext(ctx).WithFields(map[string]any{ "bucket": bucketName, }) b := d.bucketFactory.Create(bucketName) l: for { - errChan := make(chan error, 1) go func() { logger.Infof("Upgrading...") errChan <- b.Migrate( logging.ContextWithLogger(ctx, logger), migrations.WithLockRetryInterval(d.migratorLockRetryInterval), ) }() for { logger.Infof("Waiting termination") select { case <-ctx.Done(): + migrationErrors = append(migrationErrors, fmt.Errorf("migration cancelled for bucket %s: %w", bucketName, ctx.Err())) return case err := <-errChan: if err != nil { logger.Errorf("Error upgrading: %s", err) select { case <-time.After(d.migrationRetryPeriod): continue l case <-ctx.Done(): + migrationErrors = append(migrationErrors, fmt.Errorf("migration cancelled for bucket %s during retry: %w", bucketName, ctx.Err())) return } } logger.Info("Upgrade terminated") return } } } }) } wp.StopAndWait() + if len(migrationErrors) > 0 { + return fmt.Errorf("migration errors occurred: %v", migrationErrors) + } return nil }
221-245: Consider parallelizing version checks for better performance.The current implementation checks buckets sequentially, which could be slow with many buckets. Also, there are unnecessary empty lines affecting readability.
Consider this improved implementation:
func (d *Driver) HasReachMinimalVersion(ctx context.Context) (bool, error) { isUpToDate, err := d.systemStore.IsUpToDate(ctx) if err != nil { return false, fmt.Errorf("checking if system store is up to date: %w", err) } if !isUpToDate { return false, nil } - buckets, err := d.systemStore.GetDistinctBuckets(ctx) if err != nil { return false, fmt.Errorf("getting distinct buckets: %w", err) } - + type result struct { + bucket string + hasMinVersion bool + err error + } + results := make(chan result, len(buckets)) + for _, b := range buckets { - hasMinimalVersion, err := d.bucketFactory.Create(b).HasMinimalVersion(ctx) - if err != nil { - return false, fmt.Errorf("checking if bucket '%s' is up to date: %w", b, err) - } - if !hasMinimalVersion { - return false, nil - } + go func(bucketName string) { + hasMinimalVersion, err := d.bucketFactory.Create(bucketName).HasMinimalVersion(ctx) + results <- result{bucketName, hasMinimalVersion, err} + }(b) } - + + for range buckets { + r := <-results + if r.err != nil { + return false, fmt.Errorf("checking if bucket '%s' is up to date: %w", r.bucket, r.err) + } + if !r.hasMinVersion { + return false, nil + } + } return true, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yaml
📒 Files selected for processing (28)
cmd/buckets_upgrade.go(1 hunks)cmd/root.go(1 hunks)deployments/pulumi/main.go(2 hunks)deployments/pulumi/main_test.go(1 hunks)deployments/pulumi/pkg/component.go(6 hunks)docs/api/README.md(3 hunks)internal/api/common/errors.go(0 hunks)internal/api/v2/controllers_ledgers_create.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/default_bucket_test.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/driver/buckets_generated_test.go(2 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(6 hunks)internal/storage/driver/system_generated_test.go(1 hunks)internal/storage/ledger/legacy/main_test.go(1 hunks)internal/storage/module.go(2 hunks)internal/storage/system/store.go(2 hunks)pkg/client/docs/models/components/v2bulkresponse.md(1 hunks)pkg/client/formance.go(1 hunks)pkg/client/models/components/v2bulkresponse.go(2 hunks)pkg/generate/generator.go(2 hunks)pkg/testserver/server.go(3 hunks)test/e2e/app_lifecycle_test.go(2 hunks)test/migrations/upgrade_test.go(1 hunks)tools/generator/cmd/root.go(6 hunks)
💤 Files with no reviewable changes (1)
- internal/api/common/errors.go
🚧 Files skipped from review as they are similar to previous changes (20)
- internal/api/v2/controllers_ledgers_create_test.go
- test/migrations/upgrade_test.go
- cmd/buckets_upgrade.go
- cmd/root.go
- internal/storage/bucket/default_bucket_test.go
- internal/storage/ledger/legacy/main_test.go
- deployments/pulumi/main.go
- internal/api/v2/controllers_ledgers_create.go
- internal/storage/system/store.go
- deployments/pulumi/main_test.go
- internal/storage/driver/system_generated_test.go
- pkg/client/formance.go
- internal/storage/module.go
- internal/storage/driver/driver_test.go
- internal/storage/driver/buckets_generated_test.go
- internal/storage/bucket/default_bucket.go
- pkg/generate/generator.go
- internal/storage/bucket/migrations.go
- internal/storage/bucket/bucket.go
- pkg/client/models/components/v2bulkresponse.go
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/api/README.md
3802-3802: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
pkg/client/docs/models/components/v2bulkresponse.md
11-11: null
Bare URL used
(MD034, no-bare-urls)
8-8: Missing link or image reference definition: "components.v2bulkelementresult"
Reference links and images should use a label that is defined
(MD052, reference-links-images)
🔇 Additional comments (14)
pkg/testserver/server.go (3)
60-66: LGTM! Good separation of concerns
The separation of sdkClient and httpClient improves code clarity and maintainability by clearly distinguishing between the Formance SDK client and the underlying HTTP client.
226-233: LGTM! Clean initialization sequence
The initialization sequence is well-structured:
- Creates HTTP client with configured transport
- Obtains server URL from context
- Initializes SDK client with proper dependencies
263-272: LGTM! Well-designed accessor methods
The accessor methods provide clean interfaces to internal fields while maintaining encapsulation.
deployments/pulumi/pkg/component.go (2)
375-379: LGTM! Clear environment variable configuration
The environment variable setup for in-app upgrades is straightforward and follows the expected behavior.
138-160:
Channel-based initialization could cause deadlocks
The current implementation using channels could lead to deadlocks if the ApplyErr callback is not invoked.
pkg/client/docs/models/components/v2bulkresponse.md (1)
6-11: LGTM! Documentation improvements enhance error handling clarity.
The changes improve the V2BulkResponse model documentation by:
- Adding required error fields with clear types and examples
- Providing proper links to related documentation
- Following consistent formatting
🧰 Tools
🪛 Markdownlint (0.35.0)
11-11: null
Bare URL used
(MD034, no-bare-urls)
8-8: Missing link or image reference definition: "components.v2bulkelementresult"
Reference links and images should use a label that is defined
(MD052, reference-links-images)
docs/api/README.md (3)
600-603: LGTM! Example error response is well-structured.
The example error response properly demonstrates the format with all required fields:
- errorCode: Shows a valid enum value
- errorMessage: Provides a clear error description
- details: Includes a properly formatted URL
3250-3250: LGTM! New error code enhances error handling.
The addition of OUTDATED_SCHEMA to V2ErrorsEnum improves error reporting capabilities.
3804-3815:
Schema composition needs to be fixed.
The schema composition has duplicate headings which could cause confusion in generated documentation.
Apply this fix to resolve the duplicate heading:
-allOf
-
-|Name|Type|Required|Restrictions|Description|
-|---|---|---|---|---|
-|*anonymous*|object|false|none|none|
-|» data|[[V2BulkElementResult](#schemav2bulkelementresult)]|false|none|none|
-
-and
-
|Name|Type|Required|Restrictions|Description|
|---|---|---|---|---|
|*anonymous*|[V2ErrorResponse](#schemav2errorresponse)|false|none|none|Likely invalid or redundant comment.
test/e2e/app_lifecycle_test.go (2)
8-8: LGTM: Import additions are appropriate
The new imports are properly organized and necessary for the health check test implementation.
Also applies to: 15-15
281-290: Verify relationship with migration modes feature
The health check test addition seems disconnected from the PR's main objective of allowing multiple migration modes. Please clarify:
- How does the health check endpoint relate to migration modes?
- Should the health check response include migration status?
tools/generator/cmd/root.go (2)
33-34: LGTM: New flags enhance configuration capabilities
The addition of HTTP client timeout and debug flags provides better control over the application's behavior.
116-119: Verify timeout configuration impact
The HTTP client timeout configuration looks correct, but we should verify its impact on long-running operations.
Also applies to: 130-130
✅ Verification successful
Let me gather more context about the HTTP client usage and timeout configuration.
Let me gather more information about the generator's operations to understand the potential long-running operations.
Let me check the generator implementation specifically.
Based on the code analysis, I can now provide a final response about the HTTP client timeout configuration:
HTTP client timeout configuration is appropriate for the generator's operations
The generator tool uses the HTTP client primarily for bulk operations that are atomic and self-contained. The code shows that:
- The client is used for
CreateBulkoperations that are atomic (withAtomic: pointer.For(true)) - List operations are minimal, only fetching one log at a time (
PageSize: pointer.For(int64(1))) - Operations are executed in parallel with proper context handling
- The client has configurable timeout through the CLI flag with a safe default (no timeout when not specified)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential long-running operations that might be affected by the timeout
# Search for operations that might be time-sensitive
rg -A 3 "client\.(Get|Post|Put|Delete|Do)"
Length of output: 348
Script:
#!/bin/bash
# Search for HTTP client creation and configuration
rg -A 5 "http\.Client|NewClient"
# Search for timeout related configurations
rg -A 3 "Timeout|timeout"
# Look for the specific file where the timeout is being set
fd "root.go" --exec cat {}
Length of output: 48702
Script:
#!/bin/bash
# Look for the generator implementation and its operations
fd "generate" --exec cat {}
# Search for HTTP operations in the generator
rg -A 3 "client\.(Get|Post|Put|Delete|Do)" pkg/generate/
Length of output: 55932
Script:
#!/bin/bash
# Look for the generator implementation
fd "generator.go|generator_set.go" --exec cat {}
# Search for HTTP operations in the generator package
rg -A 3 "client\." pkg/generate/
Length of output: 10563
internal/storage/driver/driver.go (1)
165-165: Verify removal of minimalVersionReached channel.
Let's ensure there are no remaining references to the removed channel parameter.
✅ Verification successful
The removal of minimalVersionReached channel is safe
The git diff shows that the minimalVersionReached channel has been completely removed as part of a broader change to improve the startup probe mechanism. The changes are consistent across the codebase:
- Removed from
UpgradeAllBucketssignature and its internal logic - Removed from all
Migratefunction calls - Added a new
HasReachMinimalVersionmethod to handle version checks directly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to minimalVersionReached channel
# in bucket migrations or upgrade calls
# Search for any remaining references to minimalVersionReached
rg -i "minimalVersionReached"
# Search for Migrate function calls that might still be passing a channel
ast-grep --pattern 'Migrate($$$, make(chan struct{})$$$)'
Length of output: 89
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Search for any Migrate function calls
ast-grep --pattern 'Migrate($$$)'
# Search for any channel-related code in driver.go
rg -A 3 "chan" internal/storage/driver/driver.go
# Look for any upgrade or migration related functions
ast-grep --pattern 'func $_($$$) {
$$$
Migrate($$$)
$$$
}'
# Check the history of the file
git show HEAD -- internal/storage/driver/driver.go
Length of output: 7923
| It("should be ok when targeting health check endpoint", func() { | ||
| ret, err := testServer.GetValue().HTTPClient().Get(testServer.GetValue().ServerURL() + "/_healthcheck") | ||
| Expect(err).To(BeNil()) | ||
|
|
||
| body := make(map[string]interface{}) | ||
| Expect(json.NewDecoder(ret.Body).Decode(&body)).To(BeNil()) | ||
| Expect(body).To(Equal(map[string]any{ | ||
| storage.HealthCheckName: "OK", | ||
| })) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance health check test coverage and robustness
While the basic happy path is covered, consider the following improvements:
- Verify HTTP status code (should be 200)
- Add test cases for error scenarios
- Close response body using
defer ret.Body.Close() - Use a constant for the "OK" status string
Here's the suggested implementation:
It("should be ok when targeting health check endpoint", func() {
ret, err := testServer.GetValue().HTTPClient().Get(testServer.GetValue().ServerURL() + "/_healthcheck")
Expect(err).To(BeNil())
+ defer ret.Body.Close()
+ Expect(ret.StatusCode).To(Equal(http.StatusOK))
body := make(map[string]interface{})
Expect(json.NewDecoder(ret.Body).Decode(&body)).To(BeNil())
Expect(body).To(Equal(map[string]any{
- storage.HealthCheckName: "OK",
+ storage.HealthCheckName: storage.HealthStatusOK,
}))
})
+It("should handle health check errors", func() {
+ // Add test cases for error scenarios
+ // For example: database connection issues, service degradation, etc.
+})Committable suggestion skipped: line range outside the PR's diff.
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
from
82ab12c to
64a8593
Compare
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
from
5522a87 to
a84a33a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
docs/api/README.md (1)
3804-3815: Schema composition looks good but has a documentation issue.The schema composition for V2BulkResponse correctly combines the bulk element results with error handling fields. However, there's a documentation issue with duplicate headings that should be addressed.
Fix the duplicate heading by using a unique heading or merging the sections:
-allOf - -|Name|Type|Required|Restrictions|Description| -|---|---|---|---|---| -|*anonymous*|object|false|none|none| -|» data|[[V2BulkElementResult](#schemav2bulkelementresult)]|false|none|none| - -and - |Name|Type|Required|Restrictions|Description|pkg/generate/generator.go (2)
152-168: Consider enhancing error handling structureWhile the error handling is comprehensive, covering both HTTP-level and operation-level errors, consider these improvements:
- Define custom error types for better error handling upstream
- Handle other relevant HTTP status codes (e.g., 401, 403, 500)
- Consider aggregating multiple operation errors instead of returning on first error
Example implementation:
type BulkOperationError struct { HTTPStatus int ErrorCode string ErrorMessage string OperationErrors []OperationError } type OperationError struct { Index int Code string Description string } func (e *BulkOperationError) Error() string { return fmt.Sprintf("bulk operation failed: %s [%s]", e.ErrorMessage, e.ErrorCode) } // In Apply method: if response.HTTPMeta.Response.StatusCode != http.StatusOK { return nil, &BulkOperationError{ HTTPStatus: response.HTTPMeta.Response.StatusCode, ErrorCode: response.V2BulkResponse.ErrorCode, ErrorMessage: response.V2BulkResponse.ErrorMessage, } } var opErrors []OperationError for i, data := range response.V2BulkResponse.Data { if data.Type == components.V2BulkElementResultTypeError { opErrors = append(opErrors, OperationError{ Index: i, Code: data.V2BulkElementResultError.ErrorCode, Description: data.V2BulkElementResultError.ErrorDescription, }) } } if len(opErrors) > 0 { return nil, &BulkOperationError{ HTTPStatus: http.StatusOK, OperationErrors: opErrors, } }
Line range hint
31-171: Consider breaking down the Apply method for better maintainabilityThe Apply method is handling multiple responsibilities. Consider splitting it into smaller, focused methods:
- Extract bulk element conversion logic into separate methods per type
- Separate HTTP request handling and error processing
- Consider using the Builder pattern for bulk operations
Example refactor:
func (r Action) Apply(ctx context.Context, client *client.V2, l string) ([]components.V2BulkElementResult, error) { bulkElements, err := r.convertToBulkElements() if err != nil { return nil, fmt.Errorf("converting bulk elements: %w", err) } response, err := r.executeBulkRequest(ctx, client, l, bulkElements) if err != nil { return nil, err } return response.Data, nil } func (r Action) convertToBulkElements() ([]components.V2BulkElement, error) { // Extract element conversion logic here } func (r Action) executeBulkRequest(ctx context.Context, client *client.V2, l string, elements []components.V2BulkElement) (*components.V2BulkResponse, error) { // Extract request execution and error handling logic here }internal/storage/driver/driver.go (3)
Line range hint
165-219: LGTM! Consider enhancing error handling for worker pool tasks.The parallel bucket migration implementation using pond worker pool is well-structured. However, we might be missing potential errors from the worker pool tasks.
Consider collecting and aggregating errors from worker pool tasks:
func (d *Driver) UpgradeAllBuckets(ctx context.Context) error { // ... existing code ... + var errors []error + wp := pond.New(d.parallelBucketMigrations, len(buckets), + pond.Context(ctx), + pond.ErrorHandler(func(err error) { + errors = append(errors, err) + })) // ... existing code ... wp.StopAndWait() + if len(errors) > 0 { + return fmt.Errorf("multiple bucket upgrades failed: %v", errors) + } return nil }
221-221: Fix grammatical error in function name.The function name should be
HasReachedMinimalVersioninstead ofHasReachMinimalVersion.
221-245: Consider parallel version checks for better performance.The current implementation checks buckets sequentially. For better performance, consider using the existing worker pool to check versions in parallel.
func (d *Driver) HasReachedMinimalVersion(ctx context.Context) (bool, error) { isUpToDate, err := d.systemStore.IsUpToDate(ctx) if err != nil { return false, fmt.Errorf("checking if system store is up to date: %w", err) } if !isUpToDate { return false, nil } buckets, err := d.systemStore.GetDistinctBuckets(ctx) if err != nil { return false, fmt.Errorf("getting distinct buckets: %w", err) } + type result struct { + bucket string + hasMinVersion bool + err error + } + results := make(chan result, len(buckets)) + wp := pond.New(d.parallelBucketMigrations, len(buckets), pond.Context(ctx)) + + for _, b := range buckets { + bucket := b + wp.Submit(func() { + hasMinimalVersion, err := d.bucketFactory.Create(bucket).HasMinimalVersion(ctx) + results <- result{bucket, hasMinimalVersion, err} + }) + } + + wp.StopAndWait() + close(results) + + for r := range results { + if r.err != nil { + return false, fmt.Errorf("checking if bucket '%s' is up to date: %w", r.bucket, r.err) + } + if !r.hasMinVersion { + return false, nil + } + } return true, nil }tools/generator/cmd/root.go (1)
116-124: LGTM! Consider validating the timeout value.The HTTP client timeout implementation is good, but consider adding validation for negative timeout values.
httpClientTimeout, err := cmd.Flags().GetDuration(httpClientTimeoutFlag) if err != nil { return fmt.Errorf("failed to get http client timeout: %w", err) } +if httpClientTimeout < 0 { + return fmt.Errorf("http client timeout must be non-negative") +}deployments/pulumi/pkg/component.go (2)
159-160: Enhance upgrade mode validation.While the validation checks for invalid modes, it could be more robust by using a switch statement or a map of valid modes.
Consider this alternative implementation:
- if upgradeMode != "" && upgradeMode != UpgradeModeDisabled && upgradeMode != UpgradeModeJob && upgradeMode != UpgradeModeInApp { - return nil, fmt.Errorf("invalid upgrade mode: %s", upgradeMode) + validModes := map[UpgradeMode]bool{ + UpgradeModeDisabled: true, + UpgradeModeJob: true, + UpgradeModeInApp: true, + } + if upgradeMode != "" && !validModes[upgradeMode] { + return nil, fmt.Errorf("invalid upgrade mode: %s, valid modes are: disabled, job, in-app", upgradeMode)
Line range hint
517-552: Consider enhancing job configuration for robustness.While the basic job configuration is good, consider adding:
- Job completion deadline
- Resource limits/requests
- Back-off limit for retries
Add these configurations to improve robustness:
Spec: batchv1.JobSpecArgs{ + ActiveDeadlineSeconds: pulumi.Int(3600), // 1 hour timeout + BackoffLimit: pulumi.Int(3), // Maximum 3 retries Template: corev1.PodTemplateSpecArgs{ Spec: corev1.PodSpecArgs{ RestartPolicy: pulumi.String("OnFailure"), Containers: corev1.ContainerArray{ corev1.ContainerArgs{ Name: pulumi.String("migrate"), + Resources: &corev1.ResourceRequirementsArgs{ + Limits: pulumi.StringMap{ + "cpu": pulumi.String("500m"), + "memory": pulumi.String("512Mi"), + }, + Requests: pulumi.StringMap{ + "cpu": pulumi.String("250m"), + "memory": pulumi.String("256Mi"), + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (12)
internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/17-moves-fill-transaction-id/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/18-transactions-fill-inserted-at/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/19-transactions-fill-pcv/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/20-accounts-volumes-fill-history/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/21-transactions-metadata-fill-transaction-id/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/22-accounts-metadata-fill-address/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/23-logs-fill-memento/notes.yamlis excluded by!**/*.yamlinternal/storage/bucket/migrations/23-noop-keep-for-compatibility/notes.yamlis excluded by!**/*.yamlopenapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.yamlis excluded by!**/*.yaml
📒 Files selected for processing (28)
cmd/buckets_upgrade.go(1 hunks)cmd/root.go(2 hunks)deployments/pulumi/pkg/component.go(7 hunks)docs/api/README.md(3 hunks)internal/api/common/errors.go(0 hunks)internal/api/v2/controllers_ledgers_create.go(1 hunks)internal/api/v2/controllers_ledgers_create_test.go(1 hunks)internal/storage/bucket/bucket.go(1 hunks)internal/storage/bucket/default_bucket.go(1 hunks)internal/storage/bucket/default_bucket_test.go(1 hunks)internal/storage/bucket/migrations.go(1 hunks)internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql(1 hunks)internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql(0 hunks)internal/storage/driver/buckets_generated_test.go(2 hunks)internal/storage/driver/driver.go(2 hunks)internal/storage/driver/driver_test.go(6 hunks)internal/storage/driver/system_generated_test.go(1 hunks)internal/storage/ledger/legacy/main_test.go(1 hunks)internal/storage/module.go(2 hunks)internal/storage/system/store.go(2 hunks)pkg/client/docs/models/components/v2bulkresponse.md(1 hunks)pkg/client/formance.go(1 hunks)pkg/client/models/components/v2bulkresponse.go(2 hunks)pkg/generate/generator.go(2 hunks)pkg/testserver/server.go(3 hunks)test/e2e/app_lifecycle_test.go(2 hunks)test/migrations/upgrade_test.go(1 hunks)tools/generator/cmd/root.go(6 hunks)
💤 Files with no reviewable changes (2)
- internal/api/common/errors.go
- internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/api/v2/controllers_ledgers_create_test.go
- internal/storage/ledger/legacy/main_test.go
- cmd/buckets_upgrade.go
- test/migrations/upgrade_test.go
- internal/api/v2/controllers_ledgers_create.go
- internal/storage/bucket/default_bucket_test.go
- internal/storage/driver/system_generated_test.go
- internal/storage/bucket/migrations.go
- test/e2e/app_lifecycle_test.go
- internal/storage/system/store.go
- internal/storage/bucket/bucket.go
- internal/storage/bucket/default_bucket.go
- internal/storage/driver/buckets_generated_test.go
- internal/storage/driver/driver_test.go
- internal/storage/module.go
- pkg/client/models/components/v2bulkresponse.go
- pkg/client/formance.go
🧰 Additional context used
🪛 Markdownlint (0.35.0)
pkg/client/docs/models/components/v2bulkresponse.md
11-11: null
Bare URL used
(MD034, no-bare-urls)
8-8: Missing link or image reference definition: "components.v2bulkelementresult"
Reference links and images should use a label that is defined
(MD052, reference-links-images)
docs/api/README.md
3802-3802: null
Multiple headings with the same content
(MD024, no-duplicate-heading)
🔇 Additional comments (16)
pkg/client/docs/models/components/v2bulkresponse.md (1)
6-11: LGTM! Well-documented error handling fields.
The documentation clearly specifies the error handling fields with appropriate examples and required/optional status:
ErrorCode: Required field of type V2ErrorsEnum with example "VALIDATION"ErrorMessage: Required field of type string with example error messageDetails: Optional field with example URL
🧰 Tools
🪛 Markdownlint (0.35.0)
11-11: null
Bare URL used
(MD034, no-bare-urls)
8-8: Missing link or image reference definition: "components.v2bulkelementresult"
Reference links and images should use a label that is defined
(MD052, reference-links-images)
docs/api/README.md (2)
3250-3250: LGTM! New error code for schema versioning.
The addition of the OUTDATED_SCHEMA error code aligns well with the PR's purpose of handling multiple migration modes in the pulumi program.
600-603: LGTM! Consistent error handling example.
The example response correctly demonstrates the error handling structure with all required fields:
- errorCode
- errorMessage
- details
internal/storage/bucket/migrations/16-create-transaction-id-index-on-moves/up.sql (1)
1-1: Good use of CONCURRENTLY for zero-downtime index creation!
The concurrent index creation approach is excellent as it prevents table locks during the operation, allowing normal read/write operations to continue.
Consider adding IF NOT EXISTS to make the migration more resilient:
-create index concurrently moves_transactions_id on "{{ .Schema }}".moves(transactions_id);
+create index concurrently if not exists moves_transactions_id on "{{ .Schema }}".moves(transactions_id);Let's verify the column exists and check for any existing indexes:
pkg/generate/generator.go (2)
10-10: LGTM: Import additions are justified
The new imports are properly utilized:
pointerpackage for creating boolean pointer in bulk requestnet/httpfor HTTP status code constants in error handling
Also applies to: 18-18
146-146: LGTM: Atomic flag ensures data consistency
Setting Atomic: pointer.For(true) ensures the bulk operation is treated as an all-or-nothing transaction, which is a good practice for maintaining data consistency.
cmd/root.go (1)
42-55: LGTM! Clean implementation of the migration command.
The implementation properly initializes the logger and driver with required dependencies. The error handling is comprehensive.
tools/generator/cmd/root.go (2)
126-127: LGTM! Clean logging setup.
The logging configuration is well-structured with proper context handling.
Line range hint 130-138: LGTM! Comprehensive HTTP client configuration.
The HTTP client configuration is well-structured with proper connection pooling settings based on the number of virtual users.
pkg/testserver/server.go (3)
60-66: LGTM! Good separation of concerns.
The changes improve the structure by clearly separating the HTTP client from the SDK client and making the server URL explicitly available.
263-272: LGTM! Clean getter implementations.
The getter methods follow Go conventions and provide clear access to the internal fields.
226-233: 🛠️ Refactor suggestion
Consider adding error handling for server URL retrieval.
While the initialization sequence is logical, the httpserver.URL(s.ctx) call should be checked for potential errors or empty URL.
Consider adding error handling:
- s.serverURL = httpserver.URL(s.ctx)
+ serverURL := httpserver.URL(s.ctx)
+ if serverURL == "" {
+ return fmt.Errorf("failed to get server URL from context")
+ }
+ s.serverURL = serverURLdeployments/pulumi/pkg/component.go (4)
18-24: LGTM! Well-defined upgrade mode constants.
The UpgradeMode type and its constants are clearly defined and follow Go naming conventions.
88-88: LGTM! Consistent field addition.
The Upgrade field follows the existing pattern of using pulumix.Input for configuration values.
138-156: Potential deadlock risk in upgradeMode initialization.
The use of an unbuffered channel with pulumix.ApplyErr may lead to a deadlock if the ApplyErr callback is not invoked.
375-378: LGTM! Clear environment variable configuration.
The AUTO_UPGRADE environment variable is correctly set based on the upgrade mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
internal/storage/bucket/migrations/23-logs-fill-memento/up.sql (1)
35-37: Consider validating the constraint after data migrationThe NOT VALID constraint is safely added, but consider adding a subsequent migration to validate it once all data is confirmed correct:
ALTER TABLE logs VALIDATE CONSTRAINT memento_not_null;internal/storage/bucket/migrations/22-accounts-metadata-fill-address/up.sql (2)
41-43: Consider index optimization for the migrationWhile the NOT VALID constraint addition is safe, the join operation might benefit from an index hint:
update accounts_metadata set accounts_address = ( select /*+ INDEX(accounts accounts_pkey) */ address from accounts where accounts_metadata.accounts_seq = seq )
Line range hint
1-43: Handle orphaned records in accounts_metadataThe current migration doesn't handle cases where
accounts_metadatarecords don't have matchingaccountsrecords. Consider:
- Adding error handling for null results from the subquery
- Logging orphaned records for investigation
internal/storage/bucket/migrations/21-transactions-metadata-fill-transaction-id/up.sql (2)
41-43: Consider extracting common migration patternThis migration follows the same pattern as the other two. Consider:
- Creating a reusable migration template
- Using a migration framework that supports these patterns
Line range hint
1-43: Add monitoring for long-running migrationsFor large tables, these migrations could run for extended periods. Consider:
- Adding timing information to the notifications
- Implementing a progress monitoring system
- Adding a kill switch for emergency rollback
Example notification enhancement:
perform pg_notify('migrations-{{ .Schema }}', 'continue: ' || _batch_size || ' elapsed: ' || extract(epoch from (clock_timestamp() - batch_start)) );internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
60-62: Consider adding a separate migration for constraint validation.While the
NOT VALIDconstraint is good for immediate deployment, you should consider adding a separate migration to validate the constraint during a maintenance window. This ensures data integrity while keeping the initial migration fast.Example follow-up migration:
-- Example for a future migration ALTER TABLE transactions VALIDATE CONSTRAINT post_commit_volumes_not_null;This validation can be run later when the database load is low, as it will need to scan the entire table.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql(1 hunks)internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(1 hunks)internal/storage/bucket/migrations/21-transactions-metadata-fill-transaction-id/up.sql(1 hunks)internal/storage/bucket/migrations/22-accounts-metadata-fill-address/up.sql(1 hunks)internal/storage/bucket/migrations/23-logs-fill-memento/up.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql
🔇 Additional comments (2)
internal/storage/bucket/migrations/23-logs-fill-memento/up.sql (1)
Line range hint 1-37: Add error handling for data conversion
The current migration assumes all data conversions will succeed. Consider adding error handling:
- Verify converted data is not null before updating
- Log or handle any conversion failures
internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql (1)
60-62: LGTM! Good use of NOT VALID constraint.
Using NOT VALID is the correct approach here as it allows the constraint to be added quickly without validating existing rows, which is crucial for large tables in production environments.
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
from
d2c1e30 to
f01e014
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
deployments/pulumi/pkg/component.go (4)
18-24: Add documentation for the new type and constants.Consider adding documentation comments to explain the purpose of each upgrade mode and when to use them.
+// UpgradeMode defines the strategy for handling database migrations type UpgradeMode string const ( + // UpgradeModeDisabled indicates that no automatic migrations should be performed UpgradeModeDisabled UpgradeMode = "disabled" + // UpgradeModeJob indicates that migrations should be performed using a Kubernetes Job UpgradeModeJob UpgradeMode = "job" + // UpgradeModeInApp indicates that migrations should be performed within the application UpgradeModeInApp UpgradeMode = "in-app" )
159-160: Simplify validation using a switch statement.The validation logic can be more idiomatic using a switch statement.
-if upgradeMode != "" && upgradeMode != UpgradeModeDisabled && upgradeMode != UpgradeModeJob && upgradeMode != UpgradeModeInApp { - return nil, fmt.Errorf("invalid upgrade mode: %s", upgradeMode) +switch upgradeMode { +case "", UpgradeModeDisabled, UpgradeModeJob, UpgradeModeInApp: + // Valid modes +default: + return nil, fmt.Errorf("invalid upgrade mode: %s", upgradeMode) }
375-379: Make the upgrade mode condition more explicit.Consider making the condition more explicit to improve code readability.
-if upgradeMode == UpgradeModeInApp { +// Enable auto-upgrade only when running migrations in-app +if upgradeMode == UpgradeModeInApp {
Line range hint
520-556: Consider adding job lifecycle management.The migration job configuration looks good, but consider adding:
- TTL for completed jobs to avoid resource accumulation
- Job completion status handling
Metadata: &metav1.ObjectMetaArgs{ Namespace: namespace.Untyped().(pulumi.StringOutput), + // Clean up completed jobs after 1 day + TTLSecondsAfterFinished: pulumi.Int(86400), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
deployments/pulumi/pkg/component.go(8 hunks)internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql
🔇 Additional comments (2)
deployments/pulumi/pkg/component.go (2)
138-156: Potential deadlock risk in upgradeMode initialization.
The use of an unbuffered channel with pulumix.ApplyErr may lead to a deadlock if the ApplyErr callback is not invoked.
488-507: Verify health check probe timing configurations.
The PeriodSeconds has been increased from 10 to 60 seconds for liveness and readiness probes. This significant increase might delay the detection of unhealthy pods. Please verify:
- If 60 seconds is an appropriate interval for your use case
- If this aligns with your SLO requirements
- If this matches the behavior of other services in your cluster
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
internal/storage/bucket/migrations/11-make-stateless/up.sql (3)
Line range hint
143-171: Plan removal of temporary compatibility triggersThe comment indicates these triggers are temporary. Consider:
- Creating a tracking issue for their removal
- Documenting when they can be safely removed
- Adding tests to verify the new schema works without them
Would you like me to create a GitHub issue to track the removal of these temporary triggers?
Line range hint
266-307: Consider performance impact of transaction metadata trackingThe metadata history tracking functions perform multiple operations including selects and inserts. For high-traffic systems, this could impact performance. Consider:
- Adding appropriate indexes
- Implementing batch processing for bulk operations
Line range hint
449-471: Review advisory lock usage for reference uniquenessThe
enforce_reference_uniquenessfunction uses advisory locks which could impact concurrency. Consider:
- The lock scope is per-schema which might be too broad
- Transaction-level advisory locks could block concurrent transactions
- A unique index might be more efficient (as mentioned in the comment about the next migration)
Alternative approach:
CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS idx_transactions_reference ON transactions(ledger, reference) WHERE reference IS NOT NULL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
internal/storage/bucket/migrations/11-make-stateless/up.sql(1 hunks)
🔇 Additional comments (4)
internal/storage/bucket/migrations/11-make-stateless/up.sql (4)
Line range hint 24-33: Verify impact of dropping triggers on data integrity
The removal of multiple triggers (insert_account, update_account, etc.) could affect data integrity. Ensure that the application layer properly handles these validations now.
✅ Verification successful
Triggers are being replaced with per-ledger triggers, no data integrity concerns
The search results show that the dropped triggers (insert_account, update_account, etc.) are being replaced with more specific per-ledger triggers in the same migration file. The new triggers use the same procedures (insert_account_metadata_history, update_account_metadata_history, etc.) but are scoped to specific ledgers using a when clause.
Key evidence:
- Original triggers are dropped at the start of migration
- Same trigger functions are recreated/replaced in the migration
- New per-ledger triggers are created using dynamic SQL with conditions like
when (new.ledger = ''' || ledger.name || ''')
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any application code that might rely on these triggers
rg -A 5 'insert_account|update_account|insert_transaction|update_transaction|insert_log'
Length of output: 18386
7-7: Consider transaction isolation implications with temporary table cleanup
The on commit delete rows clause ensures proper cleanup, but be aware that this might affect concurrent transactions trying to access the same temporary table. Verify that your transaction isolation level is appropriate for your use case.
✅ Verification successful
Temporary table usage is consistent and safe across migrations
The review concern about transaction isolation can be marked as resolved. Analysis of the codebase shows that:
- All migrations using temporary tables follow the same pattern of creating, using, and explicitly dropping them within a single transaction
- Migrations 18 and 20 use temporary tables for batch processing with explicit commits, which is the correct approach
- The
on commit delete rowsclause in migration 11 aligns with the cleanup pattern used across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other temporary table usages that might be affected
rg -l 'create temporary table' | grep -v '11-make-stateless'
Length of output: 210
Script:
#!/bin/bash
# Let's check the content of these files to understand how temporary tables are used
echo "=== Content of migration 18 ==="
cat internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql
echo -e "\n=== Content of migration 20 ==="
cat internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql
Length of output: 3170
Line range hint 35-41: Review nullable column implications
Making post_commit_volumes and post_commit_effective_volumes nullable could lead to unexpected behavior in queries that don't handle NULL values properly. Consider:
- Adding NOT NULL constraints with default values if possible
- Updating application code to handle NULL cases
Line range hint 1-471: Consider migration strategy and rollback plan
This is a complex migration that touches many critical components. Consider:
- Testing the migration on a production-like dataset
- Having a clear rollback strategy
- Monitoring performance metrics during and after deployment
- Documenting the temporary solutions and their removal timeline
✅ Verification successful
Let me analyze the migration files further to verify the concerns.
Let me check the notes files for these related migrations to understand the overall migration strategy better.
Migration 11 is part of a well-structured migration sequence with proper testing
The migration is part of a larger sequence of changes that are properly ordered and tested:
- Migration 11 introduces stateless changes
- Subsequent migrations (17-23) handle data backfilling and schema updates
- Each related migration has corresponding test files (up_tests_before.sql and up_tests_after.sql)
The concerns about testing and rollback are addressed by:
- Test files present for related migrations
- Incremental changes split across multiple migrations
- Data backfilling handled in separate steps
- Each change is focused and atomic
The temporary triggers and compatibility functions are cleaned up in subsequent migrations, showing a clear migration path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing migrations that might conflict
fd -e sql . internal/storage/bucket/migrations/ | sort
Length of output: 3016
Script:
#!/bin/bash
# Check for down migrations and rollback plans
fd down.sql . internal/storage/bucket/migrations/
# Check for test files around migration 11
fd "11-make-stateless.*test" . internal/storage/bucket/migrations/
# Look for related migrations that might be part of this change
rg -l "transaction_date|accounts_address|post_commit_volumes" internal/storage/bucket/migrations/
Length of output: 1090
Script:
#!/bin/bash
# Check notes files for related migrations
cat internal/storage/bucket/migrations/19-transactions-fill-pcv/notes.yaml internal/storage/bucket/migrations/22-accounts-metadata-fill-address/notes.yaml 2>/dev/null
# Look for any documentation about the migration strategy
fd "README|MIGRATION" . internal/storage/bucket/migrations/ -t f
# Check if there are any test files that verify the migration process
fd "test|spec" . internal/storage/bucket/migrations/ -t f
Length of output: 9695
gfyrag
force-pushed
the
fix/pulumi-migrations
branch
from
35e34cd to
6e28ccf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
internal/storage/bucket/migrations/11-make-stateless/up.sql (4)
Line range hint
44-84: Consider adding indexes for volume aggregation performanceThe volume aggregation functions perform complex queries that could benefit from proper indexing.
Consider adding the following indexes to improve query performance:
+ create index idx_moves_transaction_lookup on moves(transactions_seq, ledger); + create index idx_moves_account_lookup on moves(accounts_address, asset, ledger); + create index idx_moves_effective_date on moves(effective_date, seq);
Line range hint
134-190: Document cleanup plan for temporary compatibility triggersThe compatibility triggers (
set_compat_on_move,set_compat_on_accounts_metadata,set_compat_on_transactions_metadata) are marked as temporary but lack a cleanup plan.Consider:
- Adding a TODO comment with the cleanup criteria
- Creating a follow-up migration to remove these triggers once compatibility is no longer needed
- Adding monitoring to track their usage
Line range hint
192-198: Address TODO and validate new columnsThe TODO comment indicates a postponed column type change due to locking concerns.
Would you like help with:
- Creating a separate migration for the column type change that minimizes downtime?
- Adding validation for the new columns
post_commit_volumesandinserted_at?
Line range hint
1-471: Consider breaking down the migration for safer deploymentThis migration combines multiple significant changes:
- Schema modifications
- Function updates
- Trigger changes
- Temporary compatibility layers
Consider:
- Breaking this into smaller, atomic migrations that can be deployed independently
- Adding intermediate states to validate data consistency
- Implementing feature flags to control the rollout of new functionality
- Adding monitoring for temporary compatibility layers to track their usage and ensure safe removal
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
internal/storage/bucket/migrations/11-make-stateless/up.sql(2 hunks)
🔇 Additional comments (3)
internal/storage/bucket/migrations/11-make-stateless/up.sql (3)
7-7: Improved transaction timestamp handling
Good improvements to the transaction_date() function:
- Using
on commit delete rowsensures proper cleanup of temporary data - Explicit UTC timezone conversion ensures consistent timestamp handling across the system
Also applies to: 19-19
Line range hint 31-39: Verify impact of schema changes
The changes to the moves table include significant modifications:
- Making volume columns nullable
- Adding transactions_id
- Renaming columns to plural form
These changes might impact existing queries and application code.
✅ Verification successful
Based on the search results, I can now generate the final response since I have sufficient information about the schema changes and their impact:
Schema changes are part of a broader stateless architecture transition
The changes to the moves table are part of a planned transition to a stateless architecture, as evidenced by:
- Making volume columns nullable is accompanied by a new trigger that computes these values dynamically (migration 11)
- Column renames from singular to plural form (
account_address→accounts_address) are consistently applied across the schema - The
transactions_idaddition establishes a clear relationship with the transactions table - All dependent functions and views are properly updated to handle these changes
The changes are well-structured and maintain data consistency through:
- Proper trigger updates for volume calculations
- Consistent column renaming across related functions
- Default value additions for temporal columns
- Clear migration path with updated constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential usage of old column names and nullable columns
rg -A 2 "account_address\W" --type sql
rg -A 2 "account_address_array\W" --type sql
rg -A 2 "post_commit_volumes\W" --type sql
rg -A 2 "post_commit_effective_volumes\W" --type sql
Length of output: 41782
Line range hint 449-471: Well-implemented temporary reference uniqueness check
Good implementation of reference uniqueness enforcement:
- Uses advisory locks to prevent race conditions
- Implemented as a deferred constraint for better transaction handling
- Properly scoped to non-null references only
✅ Verification successful
Temporary reference uniqueness check is correctly implemented and later replaced
The implementation is verified to be temporary as intended:
- Migration 11 adds the deferred constraint trigger
enforce_reference_uniqueness - Migration 14 creates the permanent unique index
transactions_reference2 - Migration 15 drops the temporary trigger and renames the permanent index
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing unique constraints or indexes on reference column
psql -c "\d transactions" | grep -i reference
psql -c "\di" | grep -i reference
Length of output: 273
Script:
#!/bin/bash
# Let's examine the SQL migration files for reference uniqueness constraints
fd -e sql . | xargs rg -l -i 'reference|unique|constraint'
# Get content of files containing reference-related constraints
fd -e sql . | xargs rg -A 5 -B 5 'reference.*unique|unique.*reference|constraint.*reference'
Length of output: 4084
gfyrag
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
Line range hint
8-8: Consider implementing the TODO comment for explicit advisory locks.The TODO comment about taking explicit advisory locks should be addressed to prevent concurrent migrations when the service is killed.
Consider adding this lock before the migration:
do $$ declare _batch_size integer := 1000; _date timestamp without time zone; _count integer := 0; begin + -- Take an advisory lock using schema name and migration version to prevent concurrent runs + perform pg_advisory_xact_lock(hashtext('{{.Schema}}'), 18); set search_path = '{{.Schema}}';internal/storage/driver/driver.go (3)
84-84: Consider implementing ledger caching with TTL.The TODO comment suggests caching the ledger to avoid repeated reads. This could indeed improve performance, but needs careful consideration of cache invalidation strategies.
Would you like help implementing a caching mechanism with TTL (Time To Live) to prevent stale data? I can help design a thread-safe solution using sync.Map or an LRU cache.
Line range hint
166-220: Improve error reporting from parallel migrations.While the parallel migration implementation using pond is solid, we're losing visibility into which specific buckets failed during migration. Consider collecting and returning these errors.
Here's a suggested approach using error aggregation:
func (d *Driver) UpgradeAllBuckets(ctx context.Context) error { + type bucketError struct { + bucket string + err error + } + errorsChan := make(chan bucketError, len(buckets)) wp := pond.New(d.parallelBucketMigrations, len(buckets), pond.Context(ctx)) for _, bucketName := range buckets { + bucketName := bucketName // Capture for goroutine wp.Submit(func() { logger := logging.FromContext(ctx).WithFields(map[string]any{ "bucket": bucketName, }) b := d.bucketFactory.Create(bucketName) l: for { errChan := make(chan error, 1) go func() { logger.Infof("Upgrading...") errChan <- b.Migrate( logging.ContextWithLogger(ctx, logger), migrations.WithLockRetryInterval(d.migratorLockRetryInterval), ) }() for { logger.Infof("Waiting termination") select { case <-ctx.Done(): + errorsChan <- bucketError{bucket: bucketName, err: ctx.Err()} return case err := <-errChan: if err != nil { logger.Errorf("Error upgrading: %s", err) select { case <-time.After(d.migrationRetryPeriod): continue l case <-ctx.Done(): + errorsChan <- bucketError{bucket: bucketName, err: ctx.Err()} return } } logger.Info("Upgrade terminated") return } } } }) } wp.StopAndWait() + close(errorsChan) + var errors []error + for err := range errorsChan { + if err.err != nil { + errors = append(errors, fmt.Errorf("bucket %s: %w", err.bucket, err.err)) + } + } + + if len(errors) > 0 { + return fmt.Errorf("failed to upgrade buckets: %v", errors) + } return nil }
222-246: Consider parallel version checks for better performance.The implementation is correct but performs sequential checks. For installations with many buckets, parallel checks could improve performance.
Here's a suggested parallel implementation:
func (d *Driver) HasReachMinimalVersion(ctx context.Context) (bool, error) { isUpToDate, err := d.systemStore.IsUpToDate(ctx) if err != nil { return false, fmt.Errorf("checking if system store is up to date: %w", err) } if !isUpToDate { return false, nil } buckets, err := d.systemStore.GetDistinctBuckets(ctx) if err != nil { return false, fmt.Errorf("getting distinct buckets: %w", err) } + type bucketCheck struct { + bucket string + hasMin bool + err error + } + + results := make(chan bucketCheck, len(buckets)) + for _, b := range buckets { + b := b // Capture for goroutine + go func() { + hasMinimalVersion, err := d.bucketFactory.Create(b).HasMinimalVersion(ctx) + results <- bucketCheck{ + bucket: b, + hasMin: hasMinimalVersion, + err: err, + } + }() + } + + for i := 0; i < len(buckets); i++ { + result := <-results + if result.err != nil { + return false, fmt.Errorf("checking if bucket '%s' has minimal version: %w", + result.bucket, result.err) + } + if !result.hasMin { + return false, nil + } + } return true, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql(2 hunks)internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql(1 hunks)internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql(1 hunks)internal/storage/bucket/migrations/21-transactions-metadata-fill-transaction-id/up.sql(2 hunks)internal/storage/bucket/migrations/22-accounts-metadata-fill-address/up.sql(2 hunks)internal/storage/bucket/migrations/23-logs-fill-memento/up.sql(2 hunks)internal/storage/driver/driver.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/storage/bucket/migrations/21-transactions-metadata-fill-transaction-id/up.sql
- internal/storage/bucket/migrations/22-accounts-metadata-fill-address/up.sql
- internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql
- internal/storage/bucket/migrations/23-logs-fill-memento/up.sql
🔇 Additional comments (2)
internal/storage/bucket/migrations/18-transactions-fill-inserted-at/up.sql (1)
3-3: Batch size increase looks reasonable but should be monitored.
The increase from 100 to 1000 records per batch can improve migration performance. However, ensure that this larger batch size doesn't cause excessive memory usage or lock contention in production environments, especially for tables with wide rows or high concurrent access.
Let's check for potential lock conflicts in other migrations:
✅ Verification successful
Batch size of 1000 is consistent and safe across related migrations
The batch size of 1000 is already being used successfully in adjacent migrations (17, 19, and 21) that operate on the same tables. All these migrations:
- Process data in batches of 1000 records
- Use proper transaction handling with explicit commits
- Implement progress notifications
- Include safety measures like disabling triggers when needed
- Handle locks appropriately through targeted updates
The consistent batch size across related migrations suggests this is a well-tested and reliable value for this database schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other migrations that might interact with the transactions table
# and could cause lock contention during this migration.
# Look for other migrations accessing the transactions table
rg -l "transactions" internal/storage/bucket/migrations/
Length of output: 1406
Script:
#!/bin/bash
# Let's examine the content of these migration files to understand:
# 1. The types of operations they perform (DDL vs DML)
# 2. Their locking patterns
# 3. Their batch processing approach
# Check migrations 19 and 21 as they come after our migration 18
cat internal/storage/bucket/migrations/19-transactions-fill-pcv/up.sql internal/storage/bucket/migrations/21-transactions-metadata-fill-transaction-id/up.sql
# Also check migration 17 as it's right before ours
cat internal/storage/bucket/migrations/17-moves-fill-transaction-id/up.sql
Length of output: 3802
internal/storage/bucket/migrations/20-accounts-volumes-fill-history/up.sql (1)
4-4: Batch size increase looks reasonable.
The increase from 100 to 1000 records per batch is acceptable given the safeguards in place:
- Temporary table usage controls memory footprint
- Commits after each batch prevent long-running transactions
- ON CONFLICT handling prevents data inconsistency
Let's verify the impact of this change:
✅ Verification successful
Batch size of 1000 is consistently used across similar migrations
The verification shows that all other similar migration files in the codebase also use a batch size of 1000, confirming this is an established pattern. No memory-related issues have been reported for these batch operations. The change aligns with the existing codebase standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar batch sizes in other migration files and verify memory settings
# Look for other batch size declarations in migrations
rg -g '*.sql' '_batch_size.*:=.*\d+'
# Check if there are any reported issues with similar batch sizes
gh issue list --search "batch size memory"
Length of output: 814
No description provided.