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

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.

This LGTM! Thanks Kyle

Copy link
Collaborator

@svij-sc svij-sc left a comment

Choose a reason for hiding this comment

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

Minor comment re: documentation

}

// 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.

Is this always needed to be specified by user or do we have some sane default values here?
Let's document if so?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmm for now we always want this to be supplied. However we can have some purely "config based" approach in the future, I've expanded these defs to accommodate that in the future if needed.

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.

5 participants