Skip to content

Conversation

@dlmarion
Copy link
Contributor

Closes #5062

@dlmarion dlmarion added this to the 4.0.0 milestone Nov 18, 2024
@dlmarion dlmarion self-assigned this Nov 18, 2024
Copy link
Contributor

@keith-turner keith-turner left a comment

Choose a reason for hiding this comment

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

Seems like the three metric names in this PR could use the accumulo.compaction prefix like in #5075. Like accumulo.compaction.majc.queued. Also not something for this PR, but it seems like MAJC_QUEUED and MAJC_RUNNING could be removed. Those two metrics are dupicating data the is present in per compactor RG metrics and could be derived from those.

@dlmarion
Copy link
Contributor Author

dlmarion commented Nov 21, 2024

Seems like the three metric names in this PR could use the accumulo.compaction prefix like in #5075. Like accumulo.compaction.majc.queued.

I merged into #5075 and fixed the names here.

Also not something for this PR, but it seems like MAJC_QUEUED and MAJC_RUNNING could be removed. Those two metrics are dupicating data the is present in per compactor RG metrics and could be derived from those.

CompactionCoordinator.registerMetrics registers MAJC_QUEUED and MAJC_RUNNING. I see that QueueMetrics registers COMPACTOR_JOB_PRIORITY_QUEUE_JOBS_QUEUED with a queue.id tag, which could be summed up to replace MAJC_QUEUED. I don't see a direct replacement for MAJC_RUNNING though. What did you have in mind for that?

@dlmarion
Copy link
Contributor Author

CompactionCoordinator.registerMetrics registers MAJC_QUEUED and MAJC_RUNNING. I see that QueueMetrics registers COMPACTOR_JOB_PRIORITY_QUEUE_JOBS_QUEUED with a queue.id tag, which could be summed up to replace MAJC_QUEUED. I don't see a direct replacement for MAJC_RUNNING though. What did you have in mind for that?

Ok, I see now in #5062 you said ... this information can be computed by summing idle metrics from compactors. I'm wondering if that may not be intuitive to a user to use that for running compactions given the name.

@dlmarion
Copy link
Contributor Author

CompactionCoordinator.registerMetrics registers MAJC_QUEUED and MAJC_RUNNING. I see that QueueMetrics registers COMPACTOR_JOB_PRIORITY_QUEUE_JOBS_QUEUED with a queue.id tag, which could be summed up to replace MAJC_QUEUED. I don't see a direct replacement for MAJC_RUNNING though. What did you have in mind for that?

Ok, I see now in #5062 you said ... this information can be computed by summing idle metrics from compactors. I'm wondering if that may not be intuitive to a user to use that for running compactions given the name.

Ok, using the server idle metric to gauge whether or not the Compactor is running a compaction is probably not the way to go. The process has to be idle for so long, regulated by Property.GENERAL_IDLE_PROCESS_INTERVAL, to be considered idle. This is so that the metric is not constantly flapping.

@dlmarion
Copy link
Contributor Author

I removed MAJC_QUEUED and MAJC_RUNNING. I also added some new metrics as at least one was being used in the wrong place. I look at this as some general cleanup, but I think more might be needed. Specifically, I'm thinking the metric names might need to be reviewed and corrected.

@dlmarion dlmarion merged commit a4d311c into apache:main Nov 26, 2024
8 checks passed
@dlmarion dlmarion deleted the 5062-fix-compaction-metrics branch November 26, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Metric accumulo.tserver.compactions.majc.running may not be needed anymore

2 participants