feat: Add graph.StorageMaintainer interface - BED-8353#94
feat: Add graph.StorageMaintainer interface - BED-8353#94brandonshearin wants to merge 2 commits into
Conversation
WalkthroughThe PR introduces a new storage optimization framework by defining a configurable public API in ChangesStorage Optimization Framework
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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)
graph/optimize.go (1)
7-14: ⚡ Quick winConsider adding a String() method for debugging and logging.
The
TargetStorageenum would benefit from aString()method to provide human-readable names when logging or debugging optimization operations.🎯 Proposed enhancement
const ( Nodes TargetStorage = iota Relationships ) + +func (t TargetStorage) String() string { + switch t { + case Nodes: + return "Nodes" + case Relationships: + return "Relationships" + default: + return fmt.Sprintf("TargetStorage(%d)", t) + } +}Note: This would require adding
"fmt"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@graph/optimize.go` around lines 7 - 14, Add a String() method on the TargetStorage type to map enum values to human-readable names (e.g., return "Nodes" for Nodes, "Relationships" for Relationships, and a fallback for unknown values); update the graph/optimize.go imports to include "fmt" if using fmt.Sprintf for the fallback. Implement the method with receiver (t TargetStorage) String() string and reference the constants Nodes and Relationships inside it so logging and debugging produce clear names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@drivers/pg/driver.go`:
- Around line 180-188: The Optimize method currently builds graph.OptimizeConfig
but returns nil (no-op) which falsely signals success; update Driver.Optimize to
mark it as an explicit placeholder by adding a TODO comment and returning a
clear not-implemented error (e.g., errors.New("Driver.Optimize: not
implemented") or a package-specific ErrNotImplemented) instead of nil, and
ensure any capability advertising (StorageMaintainer) is removed or gated until
the VACUUM/ANALYZE implementation using graph.OptimizeConfig is completed so
callers can distinguish implemented vs placeholder behavior.
---
Nitpick comments:
In `@graph/optimize.go`:
- Around line 7-14: Add a String() method on the TargetStorage type to map enum
values to human-readable names (e.g., return "Nodes" for Nodes, "Relationships"
for Relationships, and a fallback for unknown values); update the
graph/optimize.go imports to include "fmt" if using fmt.Sprintf for the
fallback. Implement the method with receiver (t TargetStorage) String() string
and reference the constants Nodes and Relationships inside it so logging and
debugging produce clear names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7be0ac4-5cc2-4030-8d96-a44a76e2771d
📒 Files selected for processing (3)
drivers/pg/driver.gograph/optimize.gograph/optimize_test.go
| func (s *Driver) Optimize(ctx context.Context, opts ...graph.OptimizeOption) error { | ||
| config := &graph.OptimizeConfig{} | ||
| for _, opt := range opts { | ||
| opt(config) | ||
| } | ||
|
|
||
| // PostgreSQL VACUUM/ANALYZE implementation | ||
| return nil | ||
| } |
There was a problem hiding this comment.
No-op Optimize silently reports success.
The method builds config from the options but discards it and returns nil without performing any work. This is fine as an intentional placeholder, but as written it advertises the StorageMaintainer capability while doing nothing, so callers cannot distinguish "optimized" from "not yet implemented." Consider marking the placeholder as a TODO so it surfaces in tracking and isn't mistaken for a completed implementation.
📝 Suggested placeholder marker
- // PostgreSQL VACUUM/ANALYZE implementation
- return nil
+ // TODO(BED-8353): implement PostgreSQL VACUUM/ANALYZE using config.Targets.
+ // A nil/empty config.Targets means "optimize every region" per graph.OptimizeConfig.
+ return nilWant me to open a follow-up issue to track the VACUUM/ANALYZE implementation?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@drivers/pg/driver.go` around lines 180 - 188, The Optimize method currently
builds graph.OptimizeConfig but returns nil (no-op) which falsely signals
success; update Driver.Optimize to mark it as an explicit placeholder by adding
a TODO comment and returning a clear not-implemented error (e.g.,
errors.New("Driver.Optimize: not implemented") or a package-specific
ErrNotImplemented) instead of nil, and ensure any capability advertising
(StorageMaintainer) is removed or gated until the VACUUM/ANALYZE implementation
using graph.OptimizeConfig is completed so callers can distinguish implemented
vs placeholder behavior.
brandonshearin
changed the title
| type OptimizeConfig struct { | ||
| // Targets is the set of storage regions to optimize. A nil or empty | ||
| // slice instructs the driver to optimize every region it knows about. | ||
| Targets []TargetStorage |
There was a problem hiding this comment.
Opinion: While convenient, an empty slice representing "all" supported storage targets may be misleading
| type StorageMaintainer interface { | ||
| // Optimize runs a storage maintenance pass against the regions | ||
| // identified by opts. With no options, every region the driver | ||
| // recognizes is optimized. | ||
| Optimize(ctx context.Context, opts ...OptimizeOption) error | ||
| } |
There was a problem hiding this comment.
I really dig that this interface features Options as closures. I think it has to potential to be really powerful.
For example, we could potentially pass driver specific options and only apply them when applicable. We'll want to feel something like that out a bit but here's the general idea:
m.Optimize(ctx,
graph.Targets(targets),
pg.Partitions(partitions),
pg.SkipReindex(),
)There was a problem hiding this comment.
The context of having zero options is different than declaring zero storage targets.
On one hand, m.Optimize(ctx) optimizing all DAWGS storage makes perfect sense. If you're casting that from the graph db/driver, it says "optimize everything managed by this struct."
Where the expected behavior becomes murky is something like m.Optimize(OptimizeTargets(nil)). That to me could translate to, "of everything managed by this struct only optimize none of the targets."
We'll want to spend time considering how to expose targeting individual storage targets and all storage targets.
There was a problem hiding this comment.
I think this API gracefully handles the edge case you are mentioning.
The foundational rule is: when OptimizeConfig.Targets is nil, the API should treat that as optimize all StorageTargets. The specific case you are describing is actually captured by the simple table test i made in optimize_test.go (test case #2).
This test shows that calling m.Optimize(OptimizeTargets()) will evaluate to an OptimizeConfig struct with a nil Targets property, which is what I think we want.
There was a problem hiding this comment.
I'd like to reframe my last comment as a question. Does my understanding of this edge case align with your idea of a good design here?
2 participants
Description
Resolves: BED-8353
Type of Change
Testing
make test_allwithCONNECTION_STRINGset)Screenshots (if appropriate):
Driver Impact
drivers/pg)drivers/neo4j)Checklist
go.mod/go.sumare up to date if dependencies changed