Skip to content

Conversation

zhujt20
Copy link
Contributor

@zhujt20 zhujt20 commented Sep 25, 2025

set ttl to root.__audit.log.** if audit_log_ttl_in_days is set.

@VGalaxies VGalaxies requested a review from Copilot September 25, 2025 06:09
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements TTL (Time To Live) functionality for audit log data by adding a configurable audit_log_ttl_in_days parameter. When set to a positive value, it automatically applies TTL to the audit log device path root.__audit.log.** to enable automatic cleanup of old audit logs.

  • Adds audit_log_ttl_in_days configuration parameter to CommonConfig
  • Implements TTL setting logic in DNAuditLogger when audit logs are initialized
  • Converts TTL from days to milliseconds for the SET TTL statement

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
CommonConfig.java Adds auditLogTtlInDays field with getter/setter methods
CommonDescriptor.java Loads audit_log_ttl_in_days property from configuration
DNAuditLogger.java Implements TTL setting for audit log device path during initialization

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

"SET TTL TO "
+ AUDIT_LOG_DEVICE_PATH
+ " "
+ auditLogTtlInDays * 24 * 3600 * 1000,
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The hardcoded magic numbers (24, 3600, 1000) for time conversion should be replaced with named constants or use TimeUnit for better readability and maintainability. Consider using TimeUnit.DAYS.toMillis(auditLogTtlInDays) instead.

Copilot uses AI. Check for mistakes.

Comment on lines +254 to +260
coordinator.executeForTreeModel(
statement,
SESSION_MANAGER.requestQueryId(),
sessionInfo,
"",
ClusterPartitionFetcher.getInstance(),
SCHEMA_FETCHER);
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The TTL setting operation should check the result status to ensure it was successful. If the TTL setting fails, it could lead to unexpected behavior where audit logs are not cleaned up as expected.

Suggested change
coordinator.executeForTreeModel(
statement,
SESSION_MANAGER.requestQueryId(),
sessionInfo,
"",
ClusterPartitionFetcher.getInstance(),
SCHEMA_FETCHER);
ExecutionResult ttlResult = coordinator.executeForTreeModel(
statement,
SESSION_MANAGER.requestQueryId(),
sessionInfo,
"",
ClusterPartitionFetcher.getInstance(),
SCHEMA_FETCHER);
if (ttlResult.status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
logger.error("Failed to set TTL for audit log, because {}", ttlResult.status.getMessage());
}

Copilot uses AI. Check for mistakes.

@VGalaxies VGalaxies requested a review from Copilot September 26, 2025 15:45
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants