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

API,Core: Add scan planning metrics for scanned/skipped delete manifests #5792

Merged

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Sep 19, 2022

This currently depends on #5788

@nastra nastra force-pushed the scanned-and-skipped-delete-manifest-metrics branch from adba699 to 7810f91 Compare September 19, 2022 13:32
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

LGTM, just some minor comments.

@nastra nastra force-pushed the scanned-and-skipped-delete-manifest-metrics branch 6 times, most recently from 3c2411d to 6de5ac8 Compare September 21, 2022 09:11
@nastra nastra requested a review from danielcweeks September 21, 2022 09:11
@nastra nastra force-pushed the scanned-and-skipped-delete-manifest-metrics branch from 6de5ac8 to e389a01 Compare September 21, 2022 09:19
@gaborkaszab
Copy link
Collaborator

LGTM. Looks way better with these Immutable metrics classes.

@@ -226,6 +236,16 @@ public Counter skippedDeleteFiles() {
return metricsContext().counter(SKIPPED_DELETE_FILES, MetricsContext.Unit.COUNT);
}

@Value.Derived
public Counter scannedDeleteManifests() {
return metricsContext().counter(SCANNED_DELETE_MANIFESTS, MetricsContext.Unit.COUNT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make COUNT the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created #5912 to do this

@rdblue rdblue merged commit 4fdec90 into apache:master Sep 29, 2022
@rdblue
Copy link
Contributor

rdblue commented Sep 29, 2022

Thanks, @nastra! Looks great.

@nastra nastra deleted the scanned-and-skipped-delete-manifest-metrics branch September 30, 2022 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants