Skip to content

Conversation

@parmesant
Copy link
Contributor

@parmesant parmesant commented Oct 23, 2025

Description

  • Added CRUD operations for Overview, Keystone, and Conversation in Metastore
  • Introduced a JSON error struct DetailedError

This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Summary by CodeRabbit

  • New Features
    • Extended metadata management capabilities to handle overviews, keystones, and conversations.
    • Enhanced error reporting with detailed information including timestamps, operational context, and metadata.

- Added methods for Overview, Keystone, and Conversation in Metastore
- Introduced a JSON error struct
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

This PR extends the Metastore trait with nine new asynchronous CRUD methods grouped into three metadata management categories: overviews, keystones, and conversations. Corresponding implementations are added to ObjectStoreMetastore. Additionally, a new DetailedError struct is introduced for rich error context, and the get_stream_info_helper function is made public.

Changes

Cohort / File(s) Summary
Metastore trait definition and implementation
src/metastore/metastore_traits.rs, src/metastore/metastores/object_store_metastore.rs
Added 9 new async methods across overview, keystone, and conversation categories. Overview methods return HashMap<String, Option<Bytes>> for gets and accept stream identifiers. Keystone and conversation methods return Vec<Bytes> for gets and operate on MetastoreObject trait objects. HashMap import added to support overview operations.
Error handling infrastructure
src/utils/error.rs, src/utils/mod.rs
Introduced new DetailedError struct with operation, message, stream_name, timestamp, metadata, and status_code fields. Implements helper method to convert stored u16 status code to HTTP StatusCode. Module re-exported publicly.
Logstream module visibility
src/prism/logstream/mod.rs
Changed get_stream_info_helper function from private to public async method.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The additions follow consistent CRUD patterns across multiple trait methods, reducing cognitive load. However, each implementation file requires verification of correct object store path construction, serialization logic, and error handling consistency across the nine new methods.

Possibly related PRs

  • Feat: Metastore #1424: Introduced the foundational Metastore and ObjectStoreMetastore traits that these changes extend with new metadata management operations.

Suggested labels

for next release

Suggested reviewers

  • nikhilsinhaparseable
  • praveen5959

Poem

🐰 Nine new methods hop into place,
Overviews, keystones, conversations embrace,
The metastore grows with detail and care,
While errors tell stories we're happy to share! 🎪

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is incomplete and does not adequately follow the provided template. While the description contains some information (two bullet points about what was added and completed checkboxes), it lacks critical elements required by the template: the goal of the PR, rationale for the chosen solution, and detailed explanation of key changes. The description relies on generic statements about CRUD operations and error struct introduction without explaining the underlying problem, the purpose these changes serve, or how they integrate with the broader codebase. Additionally, template guidance comments remain visible in the provided description, indicating the author did not fully replace them with substantive content. The PR objectives note also confirms that "no additional implementation details or rationale are provided in the description." The author should expand the Description section to include: a clear statement of the PR's goal and what problem it solves, the rationale for the chosen approach over alternatives, and detailed explanations of key changes with context on how they fit into the overall system. All template guidance comments should be removed and replaced with substantive content. The checklist items should be re-evaluated to ensure they accurately reflect the state of the code—particularly the claims about comments and documentation, which would benefit from specific examples or references in the description.
Title Check ❓ Inconclusive The title "Updates for Keystone" is partially related to the changeset but lacks clarity and completeness. While keystone-related methods are indeed added to the Metastore, the PR encompasses significantly more: Overview methods, Conversation methods, a new DetailedError struct, and a publicly exported helper function. The title uses vague language ("Updates for") and focuses on only one of multiple areas being modified, giving an incomplete picture of the PR's scope. A reader scanning the history would not fully understand the breadth of changes included.
✅ Passed checks (1 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc87257 and 363fe99.

📒 Files selected for processing (5)
  • src/metastore/metastore_traits.rs (2 hunks)
  • src/metastore/metastores/object_store_metastore.rs (2 hunks)
  • src/prism/logstream/mod.rs (1 hunks)
  • src/utils/error.rs (1 hunks)
  • src/utils/mod.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-06T04:26:17.191Z
Learnt from: parmesant
PR: parseablehq/parseable#1424
File: src/enterprise/utils.rs:65-72
Timestamp: 2025-09-06T04:26:17.191Z
Learning: In Parseable's metastore implementation, MetastoreError::to_detail() returns a MetastoreErrorDetail struct (not a string), which contains structured error information including operation, message, stream_name, and other contextual fields. This struct is designed to be boxed in ObjectStorageError::MetastoreError(Box<MetastoreErrorDetail>).

Applied to files:

  • src/utils/error.rs
🧬 Code graph analysis (3)
src/utils/error.rs (1)
src/prism/logstream/mod.rs (1)
  • status_code (391-404)
src/metastore/metastores/object_store_metastore.rs (2)
src/metastore/metastore_traits.rs (9)
  • get_overviews (48-48)
  • put_overview (49-53)
  • delete_overview (54-54)
  • get_keystones (57-57)
  • put_keystone (58-58)
  • delete_keystone (59-59)
  • get_conversations (62-62)
  • put_conversation (63-63)
  • delete_conversation (64-64)
src/storage/object_storage.rs (1)
  • to_bytes (1082-1086)
src/metastore/metastore_traits.rs (1)
src/metastore/metastores/object_store_metastore.rs (9)
  • get_overviews (84-98)
  • put_overview (101-108)
  • delete_overview (111-117)
  • get_keystones (120-133)
  • put_keystone (136-140)
  • delete_keystone (143-147)
  • get_conversations (150-163)
  • put_conversation (166-170)
  • delete_conversation (173-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: coverage
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
🔇 Additional comments (14)
src/utils/mod.rs (1)

21-21: LGTM!

The module declaration correctly exposes the new error module as part of the public API.

src/metastore/metastore_traits.rs (4)

19-19: LGTM!

The HashMap import is necessary for the get_overviews return type.


47-54: LGTM with design note.

The overview methods use a different signature pattern compared to keystone/conversation methods:

  • get_overviews returns a HashMap keyed by stream name (vs Vec for others)
  • put_overview requires both the object and stream name
  • delete_overview takes only the stream name (vs object for others)

This design aligns with the implementation where overviews are stored per-stream at {stream}/overview, while keystones/conversations use object IDs.


56-59: LGTM!

The keystone methods follow the established pattern used by other metadata methods (alerts, targets, etc.), using MetastoreObject for put/delete operations.


61-64: LGTM!

The conversation methods follow the same established pattern as keystones and other metadata types.

src/metastore/metastores/object_store_metastore.rs (9)

20-20: LGTM!

The HashMap import is required for the get_overviews implementation.


100-108: LGTM!

The implementation correctly serializes and stores the overview at the expected path.


110-117: LGTM!

The delete implementation is correct and follows the established pattern.


119-133: LGTM!

The implementation correctly filters keystone files from the .keystone directory, excluding conversation files (those prefixed with conv_).


135-140: LGTM!

The implementation correctly derives the path from the object ID and stores the keystone.


142-147: LGTM!

The delete implementation correctly derives the path from the object ID.


149-163: LGTM!

The implementation correctly filters conversation files from the .keystone directory using the conv_ prefix to differentiate them from keystone files.


165-170: LGTM!

The implementation correctly uses the conv_ prefix to store conversations in the .keystone directory.


172-177: LGTM!

The delete implementation correctly derives the path with the conv_ prefix, matching the put operation.

@nikhilsinhaparseable nikhilsinhaparseable merged commit 2f2b324 into parseablehq:main Oct 24, 2025
13 checks passed
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.

2 participants