chore: improve rsources service table setup procedure #4165
Conversation
atzoum
force-pushed
the
chore.rsourcesStartup
branch
3 times, most recently
from
6db13bf
to
e876c1d
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4165 +/- ##
==========================================
- Coverage 72.48% 72.47% -0.02%
==========================================
Files 383 383
Lines 55802 55822 +20
==========================================
+ Hits 40448 40455 +7
- Misses 13008 13019 +11
- Partials 2346 2348 +2 ☔ View full report in Codecov by Sentry. |
atzoum
requested review from
Sidddddarth,
lvrach,
fracasula,
cisse21,
mihir20,
BonapartePC and
achettyiitr
atzoum
force-pushed
the
chore.rsourcesStartup
branch
from
e876c1d
to
00753b0
Compare
atzoum
force-pushed
the
chore.rsourcesStartup
branch
from
00753b0
to
47d2d9d
Compare
atzoum
force-pushed
the
chore.rsourcesStartup
branch
from
47d2d9d
to
00d29e7
Compare
WalkthroughThe changes involve updates to CI/CD configurations, Makefile test commands, and source handler logic. The CI/CD pipeline now has conditional configurations for tests, the Makefile adjusts test execution based on variables, and the source handler code introduces advisory locks and a minimum pool size requirement. Test suites are refactored for better resource management and consistency. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .github/workflows/tests.yaml (3 hunks)
- Makefile (2 hunks)
- services/rsources/handler.go (4 hunks)
- services/rsources/handler_test.go (6 hunks)
- services/rsources/handler_v1_test.go (4 hunks)
- services/rsources/rsources.go (1 hunks)
Files skipped from review due to trivial changes (1)
- services/rsources/handler_test.go
Additional comments: 15
.github/workflows/tests.yaml (3)
76-82: The changes to the Warehouse Service Integration step correctly use matrix variables to run tests for specific packages. Ensure that the
make test-warehousecommand is correctly set up to accept and handle thepackagevariable.123-131: The inclusion of
services/rsourcesin the matrix and the conditional exclusion ofservices/rsourceswhen testing theservicespackage are well-implemented to avoid running redundant tests. Confirm that themake testcommand in thePackage Unitstep is correctly configured to handle these conditions.148-154: The use of
excludeandpackagevariables in the Package Unit step is a good practice for flexible test execution. Verify that themake testcommand is properly set up to handle these variables and that theexcludepatterns are correctly defined to match the intended files or directories.Makefile (1)
- 37-41: The
test-warehouse-integrationtarget correctly runs integration tests for the specified package. Ensure that thepackagevariable is always defined when this target is invoked to avoid running tests on unintended directories.services/rsources/handler.go (5)
480-480: The introduction of a constant
lockIDis a good practice for maintainability and readability.482-514: The use of
withAdvisoryLockin theinitfunction to manage concurrent setup operations is a good approach to prevent race conditions.603-609: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [528-609]
Refactoring
migrateFailedKeysTableto accept a*sql.Txensures that the migration process is transactional, which is crucial for data integrity.
813-826: The implementation of
withAdvisoryLockto acquire an advisory lock and execute a function within a transaction is a robust pattern for ensuring that critical sections of code are executed safely in a concurrent environment.482-514: Verify that the
lockIDused inwithAdvisoryLockis unique and not conflicting with other advisory locks in the system to avoid potential deadlocks or unintended behavior.services/rsources/handler_v1_test.go (4)
- 45-51: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [34-50]
The use of
BeforeEachandAfterEachinstead ofBeforeAllandAfterAllis a good practice for ensuring that each test case is run with a fresh environment, which can help prevent tests from affecting each other and improve test reliability.
45-45: In the test setup, the
createServicefunction is called without explicit error handling. Ensure that this function has internal error handling or is designed to panic in case of failure, which is acceptable in test environments.49-49: The
purgeResourcesfunction is called in theAfterEachhook without explicit error handling. Confirm that this function handles errors internally or that errors during resource cleanup do not impact the test outcomes.382-388: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [340-387]
The
BeforeEachandAfterEachhooks in the multitenant setup context are correctly used to ensure test isolation and a clean environment for each test case. This is consistent with the changes made in the single-tenant setup context and follows best practices.services/rsources/rsources.go (2)
202-203: The change to enforce a minimum
MaxPoolSizeof 2 is a good safeguard to ensure there are always enough connections available for the service to function correctly upon startup.199-201: Setting a default logger if none is provided is a good practice for ensuring that logging is always available.
| $(eval TEST_OPTIONS = -p=1 -v -failfast -shuffle=on -coverprofile=profile.out -covermode=atomic -coverpkg=./... -vet=all --timeout=15m) | ||
| endif | ||
| ifdef package | ||
| $(TEST_CMD) $(TEST_OPTIONS) $(package) && touch $(TESTFILE) || true | ||
| ifdef exclude | ||
| $(eval FILES = `go list ./$(package)/... | egrep -iv '$(exclude)'`) | ||
| $(TEST_CMD) -count=1 $(TEST_OPTIONS) $(FILES) && touch $(TESTFILE) || true | ||
| else | ||
| $(TEST_CMD) $(TEST_OPTIONS) ./$(package)/... && touch $(TESTFILE) || true | ||
| endif | ||
| else ifdef exclude | ||
| $(eval FILES = `go list ./... | egrep -iv '$(exclude)'`) | ||
| $(TEST_CMD) -count=1 $(TEST_OPTIONS) $(FILES) && touch $(TESTFILE) || true |
There was a problem hiding this comment.
The test command logic for handling package and exclude variables is correctly implemented. However, consider using $(shell ...) instead of backticks for command substitution to improve readability and maintainability in the Makefile.
| if config.Log == nil { | ||
| config.Log = logger.NewLogger().Child("rsources") | ||
| } | ||
| if config.MaxPoolSize <= 2 { | ||
| config.MaxPoolSize = 2 // minimum 2 connections in the pool for proper startup | ||
| } | ||
| var ( | ||
| localDB, sharedDB *sql.DB | ||
| err error |
There was a problem hiding this comment.
Consider checking the error returned by sql.Open before setting MaxOpenConns on localDB and sharedDB to ensure that the database connections were successfully opened.
| if config.MaxPoolSize <= 2 { | ||
| config.MaxPoolSize = 2 // minimum 2 connections in the pool for proper startup | ||
| } |
There was a problem hiding this comment.
Ensure that MaxPoolSize is not only at least 2, but also that it is not a negative value, which could cause unexpected behavior.
| return time.After(config.GetDuration("Rsources.stats.cleanup.interval", 1, time.Hour)) | ||
| } | ||
| } | ||
| sh.log.Debugf("setting up rsources tables in %s", sh.config.LocalHostname) | ||
| if err := setupTables(ctx, sh.localDB, sh.config.LocalHostname, sh.log); err != nil { | ||
|
|
||
| const lockID = 100020001 | ||
|
|
||
| if err := withAdvisoryLock(ctx, sh.localDB, lockID, func(tx *sql.Tx) error { | ||
| sh.log.Debugf("setting up rsources tables in %s", sh.config.LocalHostname) | ||
| if err := setupTables(ctx, sh.localDB, sh.config.LocalHostname, sh.log); err != nil { | ||
| return err | ||
| } | ||
| if err := migrateFailedKeysTable(ctx, tx); err != nil { | ||
| return fmt.Errorf("migrating rsources_failed_keys table: %w", err) | ||
| } | ||
| sh.log.Debugf("rsources tables setup successfully in %s", sh.config.LocalHostname) | ||
| return nil | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| if err := migrateFailedKeysTable(ctx, sh.localDB); err != nil { | ||
| return fmt.Errorf("failed to migrate rsources_failed_keys table: %w", err) | ||
| } | ||
| sh.log.Debugf("rsources tables setup successfully in %s", sh.config.LocalHostname) | ||
|
|
||
| if sh.sharedDB != nil { | ||
| sh.log.Debugf("setting up rsources tables for shared db %s", sh.config.SharedConn) | ||
| if err := setupTables(ctx, sh.sharedDB, "shared", sh.log); err != nil { | ||
| return err | ||
| } | ||
| sh.log.Debugf("rsources tables for shared db %s setup successfully", sh.config.SharedConn) | ||
| if err := withAdvisoryLock(ctx, sh.sharedDB, lockID, func(_ *sql.Tx) error { | ||
| sh.log.Debugf("setting up rsources tables for shared db %s", sh.config.SharedConn) | ||
| if err := setupTables(ctx, sh.sharedDB, "shared", sh.log); err != nil { | ||
| return err | ||
| } | ||
| sh.log.Debugf("rsources tables for shared db %s setup successfully", sh.config.SharedConn) | ||
|
|
||
| sh.log.Debugf("setting up rsources logical replication in %s", sh.config.LocalHostname) | ||
| if err := sh.setupLogicalReplication(ctx); err != nil { | ||
| return fmt.Errorf("failed to setup rsources logical replication in %s: %w", sh.config.LocalHostname, err) | ||
| sh.log.Debugf("setting up rsources logical replication in %s", sh.config.LocalHostname) | ||
| if err := sh.setupLogicalReplication(ctx); err != nil { | ||
| return fmt.Errorf("logical replication in %q: %w", sh.config.LocalHostname, err) | ||
| } | ||
| sh.log.Debugf("rsources logical replication setup successfully in %s", sh.config.LocalHostname) | ||
| return nil | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| sh.log.Debugf("rsources logical replication setup successfully in %s", sh.config.LocalHostname) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Consider refactoring the init function to break down the setup of local and shared databases into separate, smaller functions for improved readability and maintainability.
| if err != nil { | ||
| return fmt.Errorf("failed to start transaction: %w", err) | ||
| } | ||
| defer func() { _ = tx.Rollback() }() |
There was a problem hiding this comment.
The deferred rollback in withAdvisoryLock should be conditionally executed only if the commit has not been successful to avoid unnecessary rollbacks.
- defer func() { _ = tx.Rollback() }()
+ defer func() {
+ if r := recover(); r != nil || err != nil {
+ _ = tx.Rollback()
+ }
+ }()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.
| defer func() { _ = tx.Rollback() }() | |
| defer func() { | |
| if r := recover(); r != nil || err != nil { | |
| _ = tx.Rollback() | |
| } | |
| }() |
| Out: 4, | ||
| Failed: 6, | ||
| } | ||
| BeforeAll(func() { | ||
| BeforeEach(func() { | ||
| var err error | ||
| pool, err = dockertest.NewPool("") | ||
| Expect(err).NotTo(HaveOccurred()) |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [40-40]
The MaxPoolSize is set to 1 in the JobServiceConfig, which might not be sufficient for concurrent test execution. Consider increasing this value to ensure that there are enough connections available in the pool for the service to function correctly, especially during parallel test runs.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [376-376]
The MaxPoolSize is set to 1 in the JobServiceConfig for the multitenant setup, which could lead to insufficient database connections during concurrent test execution. Consider increasing this value to ensure that there are enough connections available in the pool for the service to function correctly, especially during parallel test runs.
Description
services/rsourcestests on a separate runnerLinear Ticket
resolves PIPE-488
resolves PIPE-561
Security
Summary by CodeRabbit
CI/CD Pipeline Enhancements
Testing Infrastructure
Service Reliability
JobServiceconfiguration to enforce a minimum connection pool size for stability.