-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ttl api for new trace tables #6497
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 2281fa2 in 1 minute and 36 seconds
More details
- Looked at
131
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:1441
- Draft comment:
Consider using the passedctx
instead ofcontext.Background()
to respect cancellation and timeout. - Reason this comment was not posted:
Marked as duplicate.
2. pkg/query-service/app/clickhouseReader/reader.go:1477
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The function SetTTLTracesV2 appears to be related to ClickHouse operations, specifically setting TTL for ClickHouse tables. The comment might be a general guideline, but it doesn't seem to apply to this specific change. The function is performing operations that are relevant to ClickHouse, so the comment might not be necessary here.
The comment might be addressing a broader design concern, but without specific evidence that SetTTLTracesV2 is non-ClickHouse related, the comment seems misplaced.
The function is clearly interacting with ClickHouse tables, which aligns with the purpose of the ClickHouseReader interface. The comment might be more applicable to other types of functions.
The comment does not seem relevant to the changes made in the diff, as the new function is related to ClickHouse operations. The comment should be removed.
Workflow ID: wflow_ohJB4MYGdcN7wwXl
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on b5e31e8 in 1 minute and 11 seconds
More details
- Looked at
14
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:1456
- Draft comment:
Use the context from the function parameter consistently in the Exec function.
if err := r.db.Exec(ctx, req); err != nil {
- Reason this comment was not posted:
Comment looked like it was already resolved.
2. pkg/query-service/app/clickhouseReader/reader.go:1454
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_PLHVEJAoowvlbLCv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Incremental review on bc97ac8 in 55 seconds
More details
- Looked at
22
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pkg/query-service/app/clickhouseReader/reader.go:1412
- Draft comment:
The variablettlTracesV2ResourceColdStorage
was renamed fromttlLogsV2ResourceColdStorage
to better reflect its purpose. Ensure that this change is consistently applied throughout the codebase. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/query-service/app/clickhouseReader/reader.go:1409
- Draft comment:
Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_83HrQhwB9pSVnKgX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
TTL API for the new trace tables
FOR #5713
Important
Adds
SetTTLTracesV2
inreader.go
to manage TTL for new trace tables with cold storage support, invoked whenuseTraceNewSchema
is true.SetTTLTracesV2()
inreader.go
to handle TTL for new trace tables.SetTTL()
whenuseTraceNewSchema
is true.traceTableName
,traceResourceTableV3
,signozErrorIndexTable
,signozUsageExplorerTable
,defaultDependencyGraphTable
, andtraceSummaryTable
.uuid
for transaction IDs.ttl_status
table accordingly.This description was created by for bc97ac8. It will automatically update as commits are pushed.