fix: configure sequences after import #946
Conversation
|
Warning Rate limit exceeded@gfyrag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
""" WalkthroughConcurrency control was added to the ledger state management by introducing a read-write mutex in the controller. The ledger state update and sequence resets now occur conditionally and within a transactional block, with improved error handling. End-to-end tests were updated to verify sequence restoration and clarify concurrency test logic and expectations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ControllerFacade
participant LedgerDB
Client->>ControllerFacade: handleState(ctx, dryRun, fn)
ControllerFacade->>ControllerFacade: Acquire read lock on ledger state
alt Ledger state is StateInitializing
ControllerFacade->>LedgerDB: Begin transaction
ControllerFacade->>LedgerDB: Conditionally update ledger state in DB
alt Update affected rows
ControllerFacade->>LedgerDB: Reset transaction and log sequences
end
ControllerFacade->>ControllerFacade: Execute callback function
ControllerFacade->>LedgerDB: Commit transaction
ControllerFacade->>ControllerFacade: Acquire write lock, update in-memory state
else
ControllerFacade->>ControllerFacade: Proceed without transactional locking
ControllerFacade->>ControllerFacade: Execute callback function
end
ControllerFacade-->>Client: Return result or error
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #946 +/- ##
==========================================
- Coverage 82.25% 82.25% -0.01%
==========================================
Files 141 141
Lines 7788 7883 +95
==========================================
+ Hits 6406 6484 +78
- Misses 1057 1068 +11
- Partials 325 331 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/system/state_tracker.go (1)
40-83: Consider handling NULL result from MAX operationWhen selecting the maximum ID from a table, consider handling the case where the table might be empty (resulting in NULL).
- select setval( - '"%s"."transaction_id_%d"', - ( - select max(id) from "%s".transactions where ledger = '%s' - )::bigint - ) + select setval( + '"%s"."transaction_id_%d"', + COALESCE(( + select max(id) from "%s".transactions where ledger = '%s' + ), 0)::bigint + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/system/state_tracker.go(2 hunks)test/e2e/api_ledgers_import_test.go(2 hunks)test/e2e/app_lifecycle_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (9)
test/e2e/app_lifecycle_test.go (1)
139-140: Expectation corrected to match the actual concurrency modelThe updated test now correctly expects exactly
countTransactionsadvisory locks instead ofcountTransactions + 1. This aligns with the concurrency control improvements made in the state tracker where the ledger state update and sequence resets happen in a more efficient transaction.test/e2e/api_ledgers_import_test.go (2)
352-366: Good addition: Testing sequence restoration after importThis new test verifies that transaction IDs continue in the correct sequence after an import operation. It confirms that the transaction sequence is properly reset by ensuring the next transaction ID is exactly one greater than the first transaction ID from the original ledger.
444-451: Clarified test comments and SQL filterThe updated comment now correctly explains that locking the logs table blocks the attempt to set the transaction sequence value, rather than just blocking log insertion. The SQL filter was appropriately updated to match queries containing "select setval" instead of INSERT statements, which accurately reflects what's being tested.
internal/controller/system/state_tracker.go (6)
12-12: Added sync package for concurrent access controlGood addition of the sync package to support the mutex-based concurrency control.
17-17: Added mutex for concurrency safetyAdded a read-write mutex to protect access to the ledger state, ensuring thread-safe operations in a concurrent environment.
22-26: Improved thread safety with proper lockingThe read lock ensures safe concurrent access to the ledger state. Using a local variable to store the state prevents race conditions between reading and decision-making.
40-83: Added sequence reset logic within a transactional contextThis change conditionally updates ledger state and resets both transaction and log ID sequences based on maximum existing IDs. The approach ensures that:
- Updates only happen when the ledger is in initializing state
- Sequence values are set correctly based on existing data
- All operations occur in a single transaction for atomicity
Good error handling with descriptive error messages is provided for each operation.
85-89: Improved function invocation within transactionThe function invocation is now properly wrapped within the transaction, ensuring that any operations performed by the function are part of the same atomic unit of work.
94-96: Enhanced error handling for transaction commitThe error message now includes more context about the failure, which will help with debugging issues.
gfyrag
force-pushed
the
fix/configure-sequences-after-import
branch
from
31aaae9 to
2b1be50
Compare
gfyrag
force-pushed
the
fix/configure-sequences-after-import
branch
2 times, most recently
from
3557e93 to
703c034
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/api_ledgers_import_test.go (1)
313-313: Remove focused test marker before merging.The
FItfocuses this test to run exclusively, which is typically used for debugging and should not be committed to production code.Apply this diff to restore normal test execution:
- FIt("should be ok", func(specContext SpecContext) { + It("should be ok", func(specContext SpecContext) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/controller/system/state_tracker.go(2 hunks)test/e2e/api_ledgers_import_test.go(3 hunks)test/e2e/app_lifecycle_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/app_lifecycle_test.go
- internal/controller/system/state_tracker.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/e2e/api_ledgers_import_test.go (6)
pkg/client/ledger.go (1)
Ledger(19-24)pkg/client/v2.go (1)
V2(19-21)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-25)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-68)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/components/v2transactionscursorresponse.go (1)
V2TransactionsCursorResponse(48-50)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (1)
test/e2e/api_ledgers_import_test.go (1)
461-462: LGTM! Comment and query updates accurately reflect the blocking behavior.The updated comments and SQL query correctly clarify that the test verifies blocking on sequence setting (
select setval) rather than log insertion, which aligns with the concurrency control improvements mentioned in the PR objectives.Also applies to: 467-467
| By("Checking sequence restoration by creating a new transaction with dry run", func() { | ||
| tx, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ | ||
| Ledger: createLedgerRequest.Ledger, | ||
| DryRun: pointer.For(true), | ||
| V2PostTransaction: components.V2PostTransaction{ | ||
| Postings: []components.V2Posting{{ | ||
| Source: "world", | ||
| Destination: "dst", | ||
| Asset: "USD", | ||
| Amount: big.NewInt(100), | ||
| }}, | ||
| }, | ||
| }) | ||
| Expect(err).To(BeNil()) | ||
| Expect(tx.V2CreateTransactionResponse.Data.ID.Uint64()).To(Equal(transactionsFromOriginalLedger.V2TransactionsCursorResponse.Cursor.Data[0].ID.Uint64() + 1)) | ||
| }) | ||
|
|
||
| By("Checking sequence restoration by creating a new transaction", func() { | ||
| tx, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{ | ||
| Ledger: createLedgerRequest.Ledger, | ||
| V2PostTransaction: components.V2PostTransaction{ | ||
| Postings: []components.V2Posting{{ | ||
| Source: "world", | ||
| Destination: "dst", | ||
| Asset: "USD", | ||
| Amount: big.NewInt(100), | ||
| }}, | ||
| }, | ||
| }) | ||
| Expect(err).To(BeNil()) | ||
| Expect(tx.V2CreateTransactionResponse.Data.ID.Uint64()).To(Equal(transactionsFromOriginalLedger.V2TransactionsCursorResponse.Cursor.Data[0].ID.Uint64() + 2)) | ||
| }) |
There was a problem hiding this comment.
Fix test logic for sequence restoration verification.
The sequence restoration tests are creating transactions on the original ledger (createLedgerRequest.Ledger) instead of the imported ledger (ledgerCopyName). This doesn't verify that the import process correctly restored sequences in the copied ledger.
Additionally, the test assumes transactionsFromOriginalLedger.V2TransactionsCursorResponse.Cursor.Data[0] is the first transaction by ID, but this depends on the ordering of the response data.
Apply this diff to test sequence restoration on the correct ledger and use a more reliable approach:
By("Checking sequence restoration by creating a new transaction with dry run", func() {
+ // Get the highest transaction ID from the imported ledger
+ maxTxID := uint64(0)
+ for _, tx := range transactionsFromNewLedger.V2TransactionsCursorResponse.Cursor.Data {
+ if tx.ID.Uint64() > maxTxID {
+ maxTxID = tx.ID.Uint64()
+ }
+ }
+
tx, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{
- Ledger: createLedgerRequest.Ledger,
+ Ledger: ledgerCopyName,
DryRun: pointer.For(true),
V2PostTransaction: components.V2PostTransaction{
Postings: []components.V2Posting{{
Source: "world",
Destination: "dst",
Asset: "USD",
Amount: big.NewInt(100),
}},
},
})
Expect(err).To(BeNil())
- Expect(tx.V2CreateTransactionResponse.Data.ID.Uint64()).To(Equal(transactionsFromOriginalLedger.V2TransactionsCursorResponse.Cursor.Data[0].ID.Uint64() + 1))
+ Expect(tx.V2CreateTransactionResponse.Data.ID.Uint64()).To(Equal(maxTxID + 1))
})
By("Checking sequence restoration by creating a new transaction", func() {
tx, err := Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction(ctx, operations.V2CreateTransactionRequest{
- Ledger: createLedgerRequest.Ledger,
+ Ledger: ledgerCopyName,
V2PostTransaction: components.V2PostTransaction{
Postings: []components.V2Posting{{
Source: "world",
Destination: "dst",
Asset: "USD",
Amount: big.NewInt(100),
}},
},
})
Expect(err).To(BeNil())
- Expect(tx.V2CreateTransactionResponse.Data.ID.Uint64()).To(Equal(transactionsFromOriginalLedger.V2TransactionsCursorResponse.Cursor.Data[0].ID.Uint64() + 2))
+ Expect(tx.V2CreateTransactionResponse.Data.ID.Uint64()).To(Equal(maxTxID + 2))
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/e2e/api_ledgers_import_test.go around lines 352 to 383, the test is
incorrectly creating transactions on the original ledger instead of the imported
ledger, which fails to verify sequence restoration on the copied ledger. Update
the ledger field in the CreateTransaction requests to use the imported ledger
identifier (ledgerCopyName). Also, replace the reliance on the first element of
transactionsFromOriginalLedger.V2TransactionsCursorResponse.Cursor.Data for ID
comparison with a more reliable method, such as explicitly determining the
highest existing transaction ID before the test and comparing against that to
ensure correct sequence increment.
703c034 to
4dfcf7f
Compare
No description provided.