Skip to content

Conversation

@kmontemayor2-sc
Copy link
Collaborator

@kmontemayor2-sc kmontemayor2-sc commented Jan 22, 2026

Scope of work done

Add proto definitions for custom storage main for graph store mode.

  1. [Custom Storage 1/3] Add defs for custom storage main #459
  2. [Custom Storage 2/3] Implement custom storage main #462
  3. [Custom Storage 3/3] Add validation for custom storage main #463

Where is the documentation for this feature?: N/A

Did you add automated tests or write a test plan?

Updated Changelog.md? NO

Ready for code review?: NO

Copy link
Collaborator

@mkolodner-sc mkolodner-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kyle! Generally LGTM, just a few small comments

}

// Configuration for GraphStore storage.
message GraphStoreStorageConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be renamed to GraphStore config or GraphStoreStorage config? StoreStorage seems redundant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is configuration for the "storage cluster" in graph store mode, I do get that StoreStorage has some duplication though. If we named it GraphStoreConfig I think it should also include the compute (client) config, which we don't plan to do.

We do name everything with Config here so I guess we could go against this but I am not sure that's the right call, this is a config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants