-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Add graph.StorageMaintainer interface - BED-8353 #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package graph | ||
|
|
||
| import ( | ||
| "context" | ||
| ) | ||
|
|
||
| // TargetStorage identifies a region of graph storage that an optimization | ||
| // pass may target. | ||
| type TargetStorage int | ||
|
|
||
| const ( | ||
| Nodes TargetStorage = iota | ||
| Relationships | ||
| ) | ||
|
|
||
| // OptimizeConfig is the resolved configuration for a single Optimize call. | ||
| // Drivers apply every OptimizeOption to this struct before reading it. | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opinion: While convenient, an empty slice representing "all" supported storage targets may be misleading |
||
| } | ||
|
|
||
| // OptimizeOption mutates an OptimizeConfig and is applied in the order | ||
| // passed to Optimize. | ||
| type OptimizeOption func(*OptimizeConfig) | ||
|
|
||
| // OptimizeTargets restricts the optimization pass to the given storage | ||
| // regions. Repeated calls append targets rather than replacing them. | ||
| // Calling OptimizeTargets with no arguments does not change the resolved | ||
| // target set; if no targets are configured by the time Optimize runs, the | ||
| // driver treats that as "all regions it knows about". | ||
| func OptimizeTargets(targets ...TargetStorage) OptimizeOption { | ||
| return func(c *OptimizeConfig) { | ||
| c.Targets = append(c.Targets, targets...) | ||
| } | ||
| } | ||
|
|
||
| // StorageMaintainer is an optional capability implemented by drivers that | ||
| // can perform storage maintenance. Drivers that cannot perform meaningful maintenance | ||
| // must not implement this interface; a failed type assertion is the | ||
| // signal for "this driver does not support storage maintenance". | ||
| // A non-nil error returned from Optimize signals a driver-specific failure | ||
| // during a supported call. | ||
| // | ||
| // Consumers detect support with a type assertion against a graph.Database: | ||
| // | ||
| // if m, ok := db.(graph.StorageMaintainer); ok { | ||
| // err := m.Optimize(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships)) | ||
| // ... | ||
| // } | ||
| 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 | ||
| } | ||
|
Comment on lines
+52
to
+57
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(),
)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The context of having zero options is different than declaring zero storage targets. On one hand, Where the expected behavior becomes murky is something like We'll want to spend time considering how to expose targeting individual storage targets and all storage targets.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this API gracefully handles the edge case you are mentioning. The foundational rule is: when
This test shows that calling
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package graph_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/specterops/dawgs/graph" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestOptimizeOptions(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| opts []graph.OptimizeOption | ||
| want []graph.TargetStorage | ||
| }{ | ||
| { | ||
| name: "no options yields nil Targets", | ||
| opts: nil, | ||
| want: nil, | ||
| }, | ||
| { | ||
| name: "OptimizeTargets with no args leaves Targets nil", | ||
| opts: []graph.OptimizeOption{graph.OptimizeTargets()}, | ||
| want: nil, | ||
| }, | ||
| { | ||
| name: "single call preserves order", | ||
| opts: []graph.OptimizeOption{graph.OptimizeTargets(graph.Nodes, graph.Relationships)}, | ||
| want: []graph.TargetStorage{graph.Nodes, graph.Relationships}, | ||
| }, | ||
| { | ||
| name: "repeated calls accumulate", | ||
| opts: []graph.OptimizeOption{ | ||
| graph.OptimizeTargets(graph.Nodes), | ||
| graph.OptimizeTargets(graph.Relationships), | ||
| }, | ||
| want: []graph.TargetStorage{graph.Nodes, graph.Relationships}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| var cfg graph.OptimizeConfig | ||
| for _, o := range tc.opts { | ||
| o(&cfg) | ||
| } | ||
| assert.Equal(t, tc.want, cfg.Targets) | ||
| }) | ||
| } | ||
| } |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No-op
Optimizesilently reports success.The method builds
configfrom the options but discards it and returnsnilwithout performing any work. This is fine as an intentional placeholder, but as written it advertises theStorageMaintainercapability while doing nothing, so callers cannot distinguish "optimized" from "not yet implemented." Consider marking the placeholder as aTODOso it surfaces in tracking and isn't mistaken for a completed implementation.📝 Suggested placeholder marker
Want me to open a follow-up issue to track the VACUUM/ANALYZE implementation?
🤖 Prompt for AI Agents