refactor: utility Minute handles slotting by minute
#1203
Conversation
WalkthroughThe pull request refactors the filename generation logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant S as Stream
participant T as Timestamp (NaiveDateTime)
participant M as Minute
participant P as Path Formatter
S->>T: Retrieve current timestamp
T-->>M: Convert timestamp to Minute (via From)
M->>M: Validate minute using TryFrom
M-->>S: Return slot range via to_slot()
S->>P: Format filename with date and slot range
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🪧 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
🧹 Nitpick comments (2)
src/utils/time.rs (2)
267-271: Add documentation for theblockfield and usage examples.While the struct is documented, it would be helpful to:
- Document the
blockfield explaining its purpose and valid range.- Add usage examples in the struct documentation.
/// Describes a minute block +/// +/// Represents a minute value (0-59) and provides methods for converting it to a slot range. +/// +/// # Examples +/// +/// ``` +/// use crate::utils::time::Minute; +/// +/// let minute = Minute::try_from(15).unwrap(); +/// assert_eq!(minute.to_slot(10), "10-19"); +/// ``` #[derive(Debug, Clone, Copy)] pub struct Minute { + /// The minute value (0-59) block: u32, }
273-284: Consider using a custom error type for better error messages.The current implementation returns the invalid value as the error, which doesn't provide much context. Consider creating a dedicated error type.
+#[derive(Debug, thiserror::Error)] +pub enum MinuteError { + #[error("Invalid minute value {0}, must be less than 60")] + InvalidMinute(u32), +} + impl TryFrom<u32> for Minute { - type Error = u32; + type Error = MinuteError; /// Returns a Minute if block is an acceptable minute value, else returns it as is fn try_from(block: u32) -> Result<Self, Self::Error> { if block >= 60 { - return Err(block); + return Err(MinuteError::InvalidMinute(block)); } Ok(Self { block }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/parseable/streams.rs(5 hunks)src/utils/mod.rs(0 hunks)src/utils/time.rs(3 hunks)
💤 Files with no reviewable changes (1)
- src/utils/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-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 Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (2)
src/utils/time.rs (1)
232-233: LGTM! Improved error handling and encapsulation.The refactoring improves type safety by using
try_fromfor validation and encapsulates the slot conversion logic in theMinutestruct.src/parseable/streams.rs (1)
160-164: LGTM! Consistent path format updates.The changes consistently apply the new path format and leverage the
Minutestruct for slot calculations, improving maintainability.Also applies to: 862-865, 896-899
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/time.rs (2)
282-293: Add tests for TryFrom implementation.The error handling looks good, but there are no tests for the TryFrom implementation.
Add these test cases to the test module:
#[test] fn test_minute_try_from() { assert!(Minute::try_from(0).is_ok()); assert!(Minute::try_from(59).is_ok()); assert!(Minute::try_from(60).is_err()); assert!(Minute::try_from(61).is_err()); }
303-316: Optimize to_slot method.The method could be more efficient by avoiding redundant calculations.
pub fn to_slot(self, data_granularity: u32) -> String { - let block_n = self.block / data_granularity; - let block_start = block_n * data_granularity; if data_granularity == 1 { - return format!("{block_start:02}"); + return format!("{:02}", self.block); } + let block_start = (self.block / data_granularity) * data_granularity; let block_end = (block_n + 1) * data_granularity - 1; format!("{block_start:02}-{block_end:02}") }Also, consider adding documentation for the parameters and return value:
/// Convert minutes to a slot range /// /// # Arguments /// /// * `data_granularity` - The size of each slot in minutes /// /// # Returns /// /// A string representing the slot range: /// - For granularity=1: "MM" where MM is the zero-padded minute /// - For granularity>1: "SS-EE" where SS is the slot start and EE is the slot end /// /// # Examples /// /// ``` /// let minute = Minute::try_from(15).unwrap(); /// assert_eq!(minute.to_slot(1), "15"); /// assert_eq!(minute.to_slot(10), "10-19"); /// ``` pub fn to_slot(self, data_granularity: u32) -> String {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/parseable/streams.rs(5 hunks)src/utils/time.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/parseable/streams.rs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: coverage
🔇 Additional comments (2)
src/utils/time.rs (2)
19-19: LGTM!The addition of
NaiveDateTimeto the imports is necessary for the newMinutestruct's functionality.
267-277: LGTM! Well-documented struct with clear examples.The documentation clearly explains the purpose and provides helpful examples.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/time.rs (1)
474-502: Add test cases for edge cases and error scenarios.Consider adding tests for:
- Invalid
data_granularity(0)- Overflow scenarios in slot calculations
+ #[test] + fn invalid_data_granularity() { + let minute = Minute::try_from(30).unwrap(); + assert_eq!(minute.to_slot(0), "00"); + } + + #[test] + fn overflow_in_slot_calculation() { + let minute = Minute::try_from(59).unwrap(); + assert_eq!(minute.to_slot(u32::MAX), "00"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/time.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default aarch64-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: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: coverage
🔇 Additional comments (2)
src/utils/time.rs (2)
231-236: Enhance error handling and prevent potential overflow.The current implementation has two potential issues:
- Silent error handling with
if let Ok()makes debugging harder- Unprotected multiplication of
block * data_granularitycould overflowApply this diff to address both issues:
- let mut push_prefix = |block: u32| { - if let Ok(minute) = Minute::try_from(block * data_granularity) { - let prefix = format!("{hour_prefix}minute={}/", minute.to_slot(data_granularity)); - prefixes.push(prefix); - } - }; + let mut push_prefix = |block: u32| { + match block.checked_mul(data_granularity) { + Some(minute_value) => { + match Minute::try_from(minute_value) { + Ok(minute) => { + let prefix = format!("{hour_prefix}minute={}/", minute.to_slot(data_granularity)); + prefixes.push(prefix); + } + Err(invalid_minute) => { + log::warn!( + "Invalid minute value {} generated from block {} and granularity {}", + invalid_minute, block, data_granularity + ); + } + } + } + None => { + log::warn!( + "Overflow detected when multiplying block {} with granularity {}", + block, data_granularity + ); + } + } + };
267-277: LGTM! Well-documented struct with clear examples.The documentation clearly explains the purpose and usage of the
Minutestruct.
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
Refactor
Chores
Tests
Minutestruct to validate functionality.New Features
Minutestruct for improved minute handling and slot generation.