Copilot commented on code in PR #3924:
URL: https://github.com/apache/hertzbeat/pull/3924#discussion_r2638411286
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/duckdb/DuckdbDatabaseDataStorage.java:
##########
@@ -136,11 +141,21 @@ private void startExpiredDataCleaner() {
log.info("[duckdb] start data cleaner and checkpoint...");
long expireTime;
try {
- if (NumberUtils.isParsable(expireTimeStr)) {
- expireTime = NumberUtils.toLong(expireTimeStr);
+ // Ensure no whitespace issues
+ String cleanExpireStr = expireTimeStr == null ? "" :
expireTimeStr.trim();
+ Matcher dayMatcher = DAY_PATTERN.matcher(cleanExpireStr);
+
+ if (NumberUtils.isParsable(cleanExpireStr)) {
+ expireTime = NumberUtils.toLong(cleanExpireStr);
expireTime = (ZonedDateTime.now().toEpochSecond() -
expireTime) * 1000L;
+ } else if (dayMatcher.matches()) {
+ // Strictly matched "90d" or "90D" format
+ long days = Long.parseLong(dayMatcher.group(1));
+ ZonedDateTime dateTime =
ZonedDateTime.now().minus(Duration.ofDays(days));
+ expireTime = dateTime.toEpochSecond() * 1000L;
} else {
- TemporalAmount temporalAmount =
TimePeriodUtil.parseTokenTime(expireTimeStr);
+ // Fallback to existing utility for other units (h, m, s,
etc.)
+ TemporalAmount temporalAmount =
TimePeriodUtil.parseTokenTime(cleanExpireStr);
ZonedDateTime dateTime =
ZonedDateTime.now().minus(temporalAmount);
expireTime = dateTime.toEpochSecond() * 1000L;
}
Review Comment:
The new expiration time parsing logic with DAY_PATTERN lacks test coverage.
Given that other data storage implementations in the warehouse module have test
classes (MemoryDataStorageTest, RedisDataStorageTest, TdEngineDataStorageTest),
consider adding tests to verify the new parsing behavior handles various inputs
correctly, such as "90d", "7D", whitespace variations, and edge cases like "0d"
or very large day values.
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/store/history/tsdb/duckdb/DuckdbDatabaseDataStorage.java:
##########
@@ -64,6 +66,9 @@ public class DuckdbDatabaseDataStorage extends
AbstractHistoryDataStorage {
// Ideal number of data points for charting (avoids frontend lag)
private static final int TARGET_CHART_POINTS = 800;
+ // Regex to strictly match days format, e.g., "90d", "7D". Group 1
captures the digits.
+ private static final Pattern DAY_PATTERN = Pattern.compile("^(\\d+)[dD]$");
Review Comment:
The regex pattern matches both lowercase 'd' and uppercase 'D', but
uppercase 'D' (like "90D") is already properly handled by
TimePeriodUtil.parseTokenTime() in the fallback path. This creates redundant
handling for uppercase formats. Consider restricting the pattern to only match
lowercase 'd' by changing the pattern to "^(\\d+)d$" (without the [dD]
character class), since the real enhancement is to support lowercase day
formats that TimePeriodUtil doesn't handle.
```suggestion
// Regex to strictly match lowercase days format, e.g., "90d". Group 1
captures the digits.
private static final Pattern DAY_PATTERN = Pattern.compile("^(\\d+)d$");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
