fix hot tier path#1562
Merged
nikhilsinhaparseable merged 1 commit intoparseablehq:mainfrom Mar 1, 2026
Merged
Conversation
Contributor
WalkthroughThe change modifies Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hottier.rs (1)
216-216:⚠️ Potential issue | 🔴 CriticalBug:
delete_hot_tierpath does not includetenant_id.The path construction at line 216 is inconsistent with the updated
check_stream_hot_tier_exists. For multi-tenant deployments:
- Existence check looks at:
hot_tier_path/tenant_id/stream/- Delete operates on:
hot_tier_path/stream/This will cause deletion of the wrong directory (or failure if it doesn't exist) when
tenant_idis provided.Proposed fix
pub async fn delete_hot_tier( &self, stream: &str, tenant_id: &Option<String>, ) -> Result<(), HotTierError> { if !self.check_stream_hot_tier_exists(stream, tenant_id) { return Err(HotTierValidationError::NotFound(stream.to_owned()).into()); } - let path = self.hot_tier_path.join(stream); + let path = if let Some(tenant_id) = tenant_id.as_ref() { + self.hot_tier_path.join(tenant_id).join(stream) + } else { + self.hot_tier_path.join(stream) + }; fs::remove_dir_all(path).await?; Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hottier.rs` at line 216, delete_hot_tier is building the wrong path (self.hot_tier_path.join(stream)) and must mirror check_stream_hot_tier_exists which uses hot_tier_path/tenant_id/stream; update the path construction inside delete_hot_tier to include the tenant_id when present (e.g., join tenant_id before joining stream) so the delete targets hot_tier_path/tenant_id/stream, and keep the fallback to hot_tier_path/stream only if tenant_id is absent; reference function names: delete_hot_tier, check_stream_hot_tier_exists, and fields: hot_tier_path, tenant_id, stream.
🧹 Nitpick comments (1)
src/hottier.rs (1)
597-607: The path construction logic is correct but duplicateshot_tier_file_path.The tenant-aware path logic here is identical to lines 246-255. Consider extracting a shared helper method that returns
PathBufto avoid duplication:Suggested helper extraction
+ /// Build the hot tier file path for a stream as a PathBuf + fn hot_tier_file_path_buf(&self, stream: &str, tenant_id: &Option<String>) -> PathBuf { + if let Some(tenant_id) = tenant_id.as_ref() { + self.hot_tier_path + .join(tenant_id) + .join(stream) + .join(STREAM_HOT_TIER_FILENAME) + } else { + self.hot_tier_path + .join(stream) + .join(STREAM_HOT_TIER_FILENAME) + } + } + pub fn hot_tier_file_path( &self, stream: &str, tenant_id: &Option<String>, ) -> Result<object_store::path::Path, HotTierError> { - let path = if let Some(tenant_id) = tenant_id.as_ref() { - self.hot_tier_path - .join(tenant_id) - .join(stream) - .join(STREAM_HOT_TIER_FILENAME) - } else { - self.hot_tier_path - .join(stream) - .join(STREAM_HOT_TIER_FILENAME) - }; + let path = self.hot_tier_file_path_buf(stream, tenant_id); let path = object_store::path::Path::from_absolute_path(path)?; Ok(path) } pub fn check_stream_hot_tier_exists(&self, stream: &str, tenant_id: &Option<String>) -> bool { - let path = if let Some(tenant_id) = tenant_id.as_ref() { - self.hot_tier_path - .join(tenant_id) - .join(stream) - .join(STREAM_HOT_TIER_FILENAME) - } else { - self.hot_tier_path - .join(stream) - .join(STREAM_HOT_TIER_FILENAME) - }; - path.exists() + self.hot_tier_file_path_buf(stream, tenant_id).exists() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hottier.rs` around lines 597 - 607, Extract the duplicated tenant-aware PathBuf construction into a single helper method (e.g., hot_tier_file_path(&self, stream: &str, tenant_id: Option<&str>) -> PathBuf) that uses self.hot_tier_path, STREAM_HOT_TIER_FILENAME, and joins tenant_id when present; replace the duplicated blocks (the one shown and the identical logic at lines ~246-255) to call this helper and then call .exists() on its result so the path-building logic is centralized and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/hottier.rs`:
- Line 216: delete_hot_tier is building the wrong path
(self.hot_tier_path.join(stream)) and must mirror check_stream_hot_tier_exists
which uses hot_tier_path/tenant_id/stream; update the path construction inside
delete_hot_tier to include the tenant_id when present (e.g., join tenant_id
before joining stream) so the delete targets hot_tier_path/tenant_id/stream, and
keep the fallback to hot_tier_path/stream only if tenant_id is absent; reference
function names: delete_hot_tier, check_stream_hot_tier_exists, and fields:
hot_tier_path, tenant_id, stream.
---
Nitpick comments:
In `@src/hottier.rs`:
- Around line 597-607: Extract the duplicated tenant-aware PathBuf construction
into a single helper method (e.g., hot_tier_file_path(&self, stream: &str,
tenant_id: Option<&str>) -> PathBuf) that uses self.hot_tier_path,
STREAM_HOT_TIER_FILENAME, and joins tenant_id when present; replace the
duplicated blocks (the one shown and the identical logic at lines ~246-255) to
call this helper and then call .exists() on its result so the path-building
logic is centralized and reused.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit