Skip to content
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: Add Query Trace Deleter #11026

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xiaodouchen
Copy link
Contributor

@xiaodouchen xiaodouchen commented Sep 18, 2024

Add a query trace deleter to delete all generated trace data in
query_trace_dir and avoid manually deleting them on each node of
clusters. Note that for the same query_trace_dir, the generation and
deletion of trace files should not occur concurrently to avoid potential
conflicts or issues.

Part of #9668

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 18, 2024
Copy link

netlify bot commented Sep 18, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit d219aae
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67774a1e48b58f00083d8167

@xiaodouchen
Copy link
Contributor Author

@duanmeng @xiaoxmeng @mbasmanova I have attempted to add a deletion module. Could you please review it?

Copy link

stale bot commented Dec 18, 2024

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

@stale stale bot added the stale label Dec 18, 2024
@stale stale bot closed this Jan 1, 2025
@duanmeng
Copy link
Collaborator

duanmeng commented Jan 2, 2025

@xiaodouchen Hi Xiaodou, thanks for adding this feature, it is very useful. Do you still have the capacity and interest to pursue it? If so, please reopen this PR and rebase it so we can begin the review and merge process. cc @xiaoxmeng

@xiaodouchen
Copy link
Contributor Author

@xiaodouchen Hi Xiaodou, thanks for adding this feature, it is very useful. Do you still have the capacity and interest to pursue it? If so, please reopen this PR and rebase it so we can begin the review and merge process. cc @xiaoxmeng

@duanmeng Yes, I will rebase it.

@stale stale bot removed the stale label Jan 2, 2025
"Query trace delete enabled but the trace dir is not set");

const auto deleter = trace::OperatorTraceDeleter::instance();
deleter->asyncDeleteDir(queryConfig.queryTraceDir());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean to delete the trace data by submitting a query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, delete the trace data on the node where the query is executed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kind of overkill. Normally, we store the tracing data in remote storage systems. I am adding a trace file operation tool; maybe you can add the delete function to it after it is landed. What do you think? @xiaodouchen @xiaoxmeng

Copy link
Collaborator

Choose a reason for hiding this comment

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

@xiaodouchen xiaodouchen changed the title Add Query Trace Deleter feat: Add Query Trace Deleter Jan 3, 2025
Add a query trace deleter to delete all generated trace data in
`query_trace_dir` and avoid manually deleting them on each node of
clusters. Note that for the same `query_trace_dir`, the generation and
deletion of trace files should not occur concurrently to avoid potential
conflicts or issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants