fix: set ownership for metadata streams only in the first time #4998
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DistinctValues
participant TraceListIndex
Client->>DistinctValues: Call flush()
DistinctValues->>DistinctValues: Check if _is_new
alt _is_new is true
DistinctValues->>Database: Set ownership
end
DistinctValues->>Client: Return result
Client->>TraceListIndex: Call write()
TraceListIndex->>TraceListIndex: Check if _is_new
alt _is_new is true
TraceListIndex->>Database: Set ownership
end
TraceListIndex->>Client: Return result
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/service/metadata/trace_list_index.rs (2)
86-88: Consider using a more descriptive variable name.The variable name
is_newcould be more specific, likeis_schema_newly_createdto better convey its purpose.
137-138: Enhance the comment clarity.The current comment "set ownership only in the first time" could be more descriptive.
- // set ownership only in the first time + // Set ownership only during initial schema creationsrc/service/metadata/distinct_values.rs (1)
267-271: Consider adding error handling for ownership setting.The
set_ownership_if_not_existscall could fail silently. Consider logging any errors that occur during ownership setting to aid in troubleshooting.- set_ownership_if_not_exists( - &org_id, - &format!("{}:{}", StreamType::Metadata, STREAM_NAME), - ) - .await; + if let Err(e) = set_ownership_if_not_exists( + &org_id, + &format!("{}:{}", StreamType::Metadata, STREAM_NAME), + ) + .await + { + log::error!("[DISTINCT_VALUES] Failed to set ownership: {}", e); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/service/metadata/distinct_values.rs(2 hunks)src/service/metadata/trace_list_index.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/service/metadata/distinct_values.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
src/service/metadata/trace_list_index.rs (1)
Pattern **/*.rs: You are a smart rustlang pull request reviewer.
You are going to review all the rustlang files.
Be concise, and add a brief explanation to your suggestions
Make sure the responses are not too verbose and keep the suggestions to the point i.e. actionable.
🔇 Additional comments (3)
src/service/metadata/trace_list_index.rs (1)
Line range hint 180-222: Consider potential race condition in schema initialization.
While the db_schema_init atomic flag helps prevent multiple schema checks, there's still a potential race condition between checking if db_schema.fields().is_empty() and actually creating the schema. Multiple concurrent calls could lead to multiple schema creation attempts.
Consider using a database transaction or a distributed lock to ensure atomic schema creation.
src/service/metadata/distinct_values.rs (2)
205-207: LGTM: Clean implementation of schema initialization tracking.
The is_new flag effectively tracks first-time schema creation, which is crucial for the ownership setting logic.
258-273: LGTM: Well-structured enterprise feature implementation.
Good implementation of conditional ownership setting:
- Properly gated behind enterprise feature flag
- Only sets ownership on first schema creation
- Clear documentation of intent with "set ownership only in the first time" comment
Fixes #4999
Summary by CodeRabbit
Summary by CodeRabbit
New Features
_is_newadded to track first-time schema initialization.Bug Fixes
ShutudowntoShutdown.Documentation