Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions influxdb3/src/commands/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ pub struct LastCacheConfig {
#[clap(short = 't', long = "table")]
table: String,

/// Which columns in the table to use as keys in the cache. This is a comma separated list.
/// Which columns in the table to use as keys in the cache. This is a comma separated list
///
/// Example: --key-columns "foo,bar,baz"
#[clap(long = "key-columns", value_delimiter = ',')]
Expand All @@ -158,15 +158,16 @@ pub struct LastCacheConfig {
value_columns: Option<Vec<String>>,

/// The number of entries per unique key column combination the cache will store
#[clap(long = "count")]
///
/// Higher values can increase memory usage significantly
#[clap(long = "count", default_value = "1")]
count: Option<LastCacheSize>,

/// The time-to-live (TTL) for entries in a cache. This uses a humantime form for example: --ttl "10s",
/// --ttl "1min 30sec", --ttl "3 hours"
/// The time-to-live (TTL) for entries in a cache. This uses a humantime form: "10s", "1min 30sec", "3 hours"
///
/// See the parse_duration docs for more details about acceptable forms:
/// <https://docs.rs/humantime/2.1.0/humantime/fn.parse_duration.html>
#[clap(long = "ttl")]
#[clap(long = "ttl", default_value = "4 hours")]
ttl: Option<Duration>,

/// Give a name for the cache.
Expand Down Expand Up @@ -475,7 +476,7 @@ mod tests {
"--ttl",
"1 hour",
"--count",
"5",
"15",
"bar",
]);
let super::SubCommand::LastCache(super::LastCacheConfig {
Expand All @@ -496,7 +497,7 @@ mod tests {
assert!(cache_name.is_some_and(|n| n == "bar"));
assert!(key_columns.is_some_and(|keys| keys == ["tag1", "tag2", "tag3"]));
assert!(value_columns.is_some_and(|vals| vals == ["field1", "field2", "field3"]));
assert!(count.is_some_and(|c| c == 5));
assert!(count.is_some_and(|c| c == 15));
assert!(ttl.is_some_and(|t| t.as_secs() == 3600));
}

Expand Down
4 changes: 2 additions & 2 deletions influxdb3/tests/server/configure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,8 @@ async fn api_v3_configure_last_cache_create() {
description: "Use an invalid cache size is a bad request",
db: Some(db_name),
table: Some(tbl_name),
cache_name: Some("too_big"),
count: Some(11),
cache_name: Some("too_small"),
count: Some(0),
expected: StatusCode::BAD_REQUEST,
..Default::default()
},
Expand Down
2 changes: 1 addition & 1 deletion influxdb3_cache/src/last_cache/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1236,7 +1236,7 @@ mod tests {
Some("test_cache_3"),
Option::<&[&str]>::Some(&[]),
Some(&["f2", "time"]),
LastCacheSize::new(10).unwrap(),
LastCacheSize::new(100).unwrap(),
LastCacheTtl::from_secs(500),
)
.await
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ expression: catalog.snapshot()
2,
3
],
"n": 10,
"n": 100,
"ttl": 500
}
]
Expand Down
2 changes: 1 addition & 1 deletion influxdb3_catalog/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ pub enum CatalogError {
#[error("failed to parse trigger from {}", trigger_spec)]
ProcessingEngineTriggerSpecParseError { trigger_spec: String },

#[error("last cache size must be from 1 to 10")]
#[error("last cache size must be greater than 0")]
InvalidLastCacheSize,

#[error("failed to parse trigger from {trigger_spec}{}", .context.as_ref().map(|context| format!(": {context}")).unwrap_or_default())]
Expand Down
7 changes: 2 additions & 5 deletions influxdb3_catalog/src/log/versions/v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -380,18 +380,15 @@ pub enum LastCacheValueColumnsDef {
AllNonKeyColumns,
}

/// The maximum allowed size for a last cache
pub const LAST_CACHE_MAX_SIZE: usize = 10;

/// The size of the last cache
///
/// Must be between 1 and [`LAST_CACHE_MAX_SIZE`]
/// Must be greater than 0
#[derive(Debug, Serialize, Eq, PartialEq, Clone, Copy)]
pub struct LastCacheSize(pub(crate) usize);
Comment on lines +385 to 387
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a textbook use case for NonZeroUsize, i.e.,

/// Must be greater than 0
#[derive(Debug, Serialize, Eq, PartialEq, Clone, Copy)]
pub struct LastCacheSize(pub(crate) NonZeroUsize);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This looks like it'll be more involved given implementations across the DB will have to be updated; I can do that update, would want to do it in a separate chore though if that's alright.

If you've reconsidered and do think it should be blocking, let me know and I'll make the update.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No worries. We can refactor this later


impl LastCacheSize {
pub fn new(size: usize) -> Result<Self> {
if size == 0 || size > LAST_CACHE_MAX_SIZE {
if size == 0 {
Err(CatalogError::InvalidLastCacheSize)
} else {
Ok(Self(size))
Expand Down