Add scaffolding for XMTPD DB prune / data retention #772
Conversation
WalkthroughA new "prune" command-line tool and supporting infrastructure were added, including its Go implementation, configuration, validation, Docker packaging, and development script. The build workflow was updated to build and push the new "xmtpd-prune" Docker image. Supporting code for database connection and a stub executor for pruning were also introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PruneCLI as Prune CLI (main.go)
participant Config as Config/Validation
participant Logger
participant DB as Database Connector
participant Executor as Prune Executor
User->>PruneCLI: Run prune command with options
PruneCLI->>Config: Parse and validate PruneOptions
Config-->>PruneCLI: Return validation result
PruneCLI->>Logger: Initialize logger
PruneCLI->>DB: ConnectToDB with options
DB-->>PruneCLI: Return DB handle
PruneCLI->>Executor: NewPruneExecutor(ctx, logger, db)
PruneCLI->>Executor: Run()
Executor-->>PruneCLI: Return result
PruneCLI-->>User: Exit (success or error)
Possibly related PRs
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ 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
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
pkg/db/pgx.go (2)
161-166: Consider improving function signature readabilityThe parameter arrangement in this function signature is slightly inconsistent. The
waitForDBandstatementTimeoutparameters are on the same line, whilenamespaceis on the same line without a line-breaking comma.Consider rearranging the parameters to improve readability:
func ConnectToDB( ctx context.Context, logger *zap.Logger, dsn string, - waitForDB time.Duration, statementTimeout time.Duration, namespace string, + waitForDB time.Duration, + statementTimeout time.Duration, + namespace string, ) (*sql.DB, error) {
172-174: Consider validating the namespace if providedWhile the function allows for an optional namespace, it doesn't validate it when provided, unlike the
createNamespacefunction which callsisValidNamespace.Add validation to ensure the provided namespace is valid:
if namespace != "" { + if err := isValidNamespace(namespace); err != nil { + return nil, err + } config.ConnConfig.Database = namespace }dev/prune (1)
1-7: Add checks for prerequisites and error handlingThis script assumes that the
dev/local.envfile exists and that git is correctly configured with tags. Whileset -euhelps with error detection, more explicit checks could improve the user experience.Consider adding checks for the environment file and providing fallback for version information:
#!/bin/bash set -eu +# Check if local.env exists +if [ ! -f dev/local.env ]; then + echo "Error: dev/local.env not found. Please create it before running this script." + exit 1 +fi . dev/local.env +# Get version with fallback +VERSION=$(git describe HEAD --tags --long 2>/dev/null || echo "unknown") + -go run -ldflags="-X main.Version=$(git describe HEAD --tags --long)" cmd/prune/main.go "$@" +go run -ldflags="-X main.Version=$VERSION" cmd/prune/main.go "$@"dev/docker/Dockerfile-prune (1)
1-35: Well-structured Dockerfile following best practicesThe multi-stage build approach effectively minimizes the final image size while properly configuring the environment for the pruning tool.
Consider providing a default value for the VERSION build arg to match the "unknown" fallback used in the build command:
-ARG VERSION=unknown +ARG VERSION=unknowncmd/prune/main.go (2)
29-34: Consider a fallback version instead of failingInstead of failing when Version is not set by build or environment, consider providing a fallback value to improve robustness.
if Version == "" { Version = os.Getenv("VERSION") if Version == "" { - fatal("Could not determine version") + Version = "development" + logger.Warn("Could not determine version, using default", zap.String("version", Version)) } }
69-71: Consider using structured logging for fatal errorsThe current implementation uses standard log.Fatalf, but using the structured logger would provide consistency with the rest of the application's logging.
func fatal(msg string, args ...any) { - log.Fatalf(msg, args...) + if logger != nil { + logger.Fatal(fmt.Sprintf(msg, args...)) + } else { + log.Fatalf(msg, args...) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build-xmtpd.yml(2 hunks)cmd/prune/main.go(1 hunks)dev/docker/Dockerfile-prune(1 hunks)dev/prune(1 hunks)pkg/config/pruneOptions.go(1 hunks)pkg/config/validation.go(1 hunks)pkg/db/pgx.go(1 hunks)pkg/prune/prune.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
pkg/config/validation.go (2)
pkg/config/pruneOptions.go (1)
PruneOptions(3-6)pkg/db/queries/db.go (1)
New(19-21)
pkg/config/pruneOptions.go (1)
pkg/config/options.go (2)
DbOptions(24-32)LogOptions(73-76)
cmd/prune/main.go (5)
pkg/config/pruneOptions.go (1)
PruneOptions(3-6)pkg/config/validation.go (1)
ValidatePruneOptions(95-125)pkg/utils/log.go (1)
BuildLogger(9-40)pkg/db/pgx.go (1)
ConnectToDB(161-184)pkg/prune/prune.go (1)
NewPruneExecutor(16-26)
🔇 Additional comments (5)
.github/workflows/build-xmtpd.yml (2)
55-56: LGTM: Good addition for the new prune imageThe conditional logic to select the appropriate Dockerfile for the "xmtpd-prune" image is well-integrated with the existing pattern.
15-15:Details
✅ Verification successful
Confirm that the Dockerfile-prune exists and functions properly
The addition of "xmtpd-prune" to the matrix strategy assumes that there's a corresponding
Dockerfile-prunein./dev/docker/directory.Verify that the Dockerfile exists and contains the proper configuration:
🏁 Script executed:
#!/bin/bash # Check if Dockerfile-prune exists if [ -f ./dev/docker/Dockerfile-prune ]; then echo "Dockerfile-prune exists:" echo "----------------------" cat ./dev/docker/Dockerfile-prune | head -10 echo "..." else echo "ERROR: Dockerfile-prune not found in ./dev/docker/" fiLength of output: 467
🏁 Script executed:
#!/bin/bash echo "ENTRYPOINT/CMD lines in dev/docker/Dockerfile-prune:" rg "(ENTRYPOINT|CMD)" -n dev/docker/Dockerfile-prune || true echo "" echo "---- Last 20 lines of dev/docker/Dockerfile-prune ----" tail -n 20 dev/docker/Dockerfile-pruneLength of output: 862
Dockerfile-prune exists and is correctly configured
- Confirmed
dev/docker/Dockerfile-pruneis present.- Includes a two-stage build (
builderandalpine:3.21) and sets
ENTRYPOINT ["/usr/bin/xmtpd-prune"]as expected.pkg/config/validation.go (1)
95-125: LGTM - Clean implementation of ValidatePruneOptionsThe validation function correctly checks for the required DB.WriterConnectionString field and follows the established pattern from ValidateServerOptions.
pkg/prune/prune.go (1)
1-31: Good foundation for the pruning executorThe Executor struct is well-designed with proper encapsulation of dependencies (context, logger, and database connection). The structure provides a clean starting point for implementing the actual pruning logic in future PRs.
Note that the Run method is currently just a placeholder. In future PRs, this will need to be implemented with the actual pruning logic, including data retention policies, transaction management, and error handling.
cmd/prune/main.go (1)
1-68: Well-orchestrated command-line tool implementationThe main package effectively coordinates configuration parsing, validation, database connection setup, and pruning execution with proper error handling at each step.
Consider adding graceful shutdown handling in a future PR to ensure proper cleanup of resources (especially database connections) when the process is terminated.
#!/bin/bash # Check for other database connection management in the codebase # to ensure consistency with existing patterns # Search for signal handling patterns rg -A 5 "os.Signal|signal.Notify" --type go
Add database pruning infrastructure to XMTPD by implementing
|
pkg/config/validation.go
Outdated
|
|
||
| func ValidatePruneOptions(options PruneOptions) error { | ||
| missingSet := make(map[string]struct{}) | ||
| customSet := make(map[string]struct{}) |
There was a problem hiding this comment.
Do you plan on using customSet? Otherwise it can be deleted.
Add scaffolding for XMTPD database pruning by implementing prune command-line tool, configuration, and Docker support
xmtpd-prunecommand-line tool in main.go with configuration and database connection handling📍Where to Start
Start with the main entry point in main.go which sets up the command-line interface, configuration parsing, and execution flow for the pruning tool.
Macroscope summarized 6566e91.
Summary by CodeRabbit
New Features
Documentation