Add Metrics for CompactionJobPriority Queues by ddanielr · Pull Request #3551 · apache/accumulo

ddanielr · 2023-06-28T15:00:51Z

Adds the following metrics for cjpq to solve #3474:

Number of Queues
Queue Size
Number of Queued Jobs
Number of Dequeued Jobs
Number of Rejected Jobs

This is using the Thread polling method of updating metric values.

Looking for feedback on metric naming & prefixes, and implementation.

Adds the following metrics for cjpq: * Number of Queues * Queue Size * Number of Queued Jobs * Number of Dequeued Jobs * Number of Rejected Jobs

Removed bad line of code that was left over from a poor merge conflict resolution

EdColeman · 2023-06-28T22:05:35Z

In general - why are the metric changes being testing in BulkNewIT? Would it be better if the test was a stand-alone test of the metrics? If a package does not exist, in may be desirable to have a metrics specific sub-package for metric IT tests so that they are easier to find?

Applies feedback from PR review. * Changes metric registration to register via queue name, rather than by queue object. * * This helps avoid metric issues when the queues are created & deleted. * Removes unnessessary Atomic var for tracking queued Jobs. * Fixes naming scheme issues.

Switch from size test to looking up all metrics via queue ID.

ddanielr · 2023-07-07T23:14:19Z

In general - why are the metric changes being testing in BulkNewIT? Would it be better if the test was a stand-alone test of the metrics? If a package does not exist, in may be desirable to have a metrics specific sub-package for metric IT tests so that they are easier to find?

I was testing this in BulkNewIT as I knew that the compaction operations would trigger and metrics would be generated.
This was done as a convenience to get these metrics generated quickly.

However, I need to add more deliberate tests for these metrics to ensure the gauges are being set to correct values.
Right now it's just collecting metrics with the specified prefix and putting them into the log.

I'm not sure about how to best organize the metric tests so I'd be happy to discuss the best options there.

Added formatting steps to formatString and included test class with string examples.

* Switched CompactionJobQueues to a Final * Cleaned up anonymous functions in the update method.

* Fixes the queue count metrics * Switches metric names to dot notation * Removes test changes from BulkNewIT * Adds CompactionPriorityQueueMetricsIT * Adds User defined CompactionResourceGroup override to MAC * Adds new Lowest Job Priority Metric

ddanielr · 2023-07-18T08:39:00Z

Currently the new test added in (CompactionPriorityQueueMetricsIT.java) fails when run.
I believe the check condition is correct and that we may have some unintended behaviors happening when multiple compaction jobs are being submitted to the same queue.

Also should all references to queue be swapped for compaction group? The jobs still do function in a queue structure.

* Refactors IT test to create additional tablets and properly generate compaction jobs without having selected file collisions. * Adds configurable property to change CompactionQueue size and test rejectedJobs. See apache#3635 for more details. * Adds comparision checks in the IT to check metric values against known datapoints.

ddanielr · 2023-07-19T06:28:35Z

Currently the new test added in (CompactionPriorityQueueMetricsIT.java) fails when run. I believe the check condition is correct and that we may have some unintended behaviors happening when multiple compaction jobs are being submitted to the same queue.

Also should all references to queue be swapped for compaction group? The jobs still do function in a queue structure.

Discussed this with @keith-turner and discovered the issue was with my setup of the tablets and split points.
Test was corrected to have more tablets than queue slots and it is now passing.

Additional checks were added to validate specific metrics values.

#3635 was also created to eventually replace a property that was added for setting a PriorityQueue's maxSize.

* Removes unnecessary comments * Switches metric names to use `queue` vs `cjpq` * Adds final modifier to queueSize var

keith-turner · 2023-07-20T01:53:16Z

+
+    boolean sawQueues = false;
+    // An empty queue means that the last known value is the most recent.
+    while (!queueMetrics.isEmpty()) {


Would it make sense to also check the dequeued count in this part of the test?

I tried doing that, but the problem with dequeued is that once all the jobs are processed the queue is removed so dequeued returns 0.
Due to the 3 second polling rate, it's hard to identify that condition quickly enough in the test.

If we wanted to check dequeued jobs then that should be a separate test with a large queue size and a slow compaction (+1 second runtime per compaction)

keith-turner · 2023-07-20T01:57:38Z

+  }
+
+  @Test
+  public void testQueueMetrics() throws Exception {


Would be nice to test metrics from multiple queues, just to ensure the data for queue is independent. Could be a follow in issue.

keith-turner · 2023-07-20T01:59:07Z

@ddanielr are you ready to merge this?

ddanielr · 2023-07-20T02:00:14Z

Yes. I believe it's in a good state.

…ction/queue/CompactionJobPriorityQueue.java

keith-turner · 2023-07-20T02:11:36Z

ok, I will merge if the build passes

Add Metrics for CompactionJobPriority Queues

cc61007

Adds the following metrics for cjpq: * Number of Queues * Queue Size * Number of Queued Jobs * Number of Dequeued Jobs * Number of Rejected Jobs

ddanielr force-pushed the 3474-add-metrics branch from fcbfe19 to cc61007 Compare June 28, 2023 15:21

EdColeman reviewed Jun 28, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/accumulo/core/metrics/MetricsUtil.java Outdated

Fix BulkNewIT test

54fe656

Removed bad line of code that was left over from a poor merge conflict resolution

keith-turner reviewed Jun 28, 2023

View reviewed changes

keith-turner mentioned this pull request Jul 3, 2023

Explore removing queued jobs when planner emits zero jobs. #3528

Closed

ddanielr added 4 commits July 7, 2023 21:30

Merge branch 'elasticity' into 3474-add-metrics

9d3dd18

Address formatting issues

d98ef7d

Update queue metrics to use new access pattern

f9b3e5e

Switch from size test to looking up all metrics via queue ID.

keith-turner reviewed Jul 7, 2023

View reviewed changes

Comment thread .../manager/src/main/java/org/apache/accumulo/manager/compaction/queue/CompactionJobQueues.java Outdated

Solves race condition with duplicate .get() calls

e5b121c

ddanielr added 3 commits July 10, 2023 20:08

Merge branch 'elasticity' into 3474-add-metrics

d2234b5

Adds formatting steps to MetricUtil

7ee44b6

Added formatting steps to formatString and included test class with string examples.

General cleanup

03c54b8

* Switched CompactionJobQueues to a Final * Cleaned up anonymous functions in the update method.

keith-turner reviewed Jul 12, 2023

View reviewed changes

keith-turner mentioned this pull request Jul 15, 2023

Generate metrics about running compactions. #3619

Closed

ddanielr added 3 commits July 18, 2023 08:01

Add new compaction metrics test

b674c5e

* Fixes the queue count metrics * Switches metric names to dot notation * Removes test changes from BulkNewIT * Adds CompactionPriorityQueueMetricsIT * Adds User defined CompactionResourceGroup override to MAC * Adds new Lowest Job Priority Metric

Merge branch 'elasticity' into 3474-add-metrics

869d127

Fixes test to use new group vs queue name

6586ce7

ddanielr mentioned this pull request Jul 19, 2023

Add configurable Property to set a CompactionJobPriorityQueue's maxSize #3635

Closed

ddanielr added 2 commits July 19, 2023 05:47

Merge branch 'elasticity' into 3474-add-metrics

3861cee

ddanielr marked this pull request as ready for review July 19, 2023 06:23

dlmarion reviewed Jul 19, 2023

View reviewed changes

Addresses PR Feedback

da1bcd3

* Removes unnecessary comments * Switches metric names to use `queue` vs `cjpq` * Adds final modifier to queueSize var

Removes old comment

8cfa533

keith-turner approved these changes Jul 20, 2023

View reviewed changes

Update server/manager/src/main/java/org/apache/accumulo/manager/compa…

285d663

…ction/queue/CompactionJobPriorityQueue.java

keith-turner merged commit 6e745d1 into apache:elasticity Jul 20, 2023

ddanielr deleted the 3474-add-metrics branch July 25, 2023 17:29

dlmarion mentioned this pull request Nov 28, 2023

Emit metrics for compaction priority queues #3474

Closed

ddanielr mentioned this pull request Nov 28, 2023

Add test for returning metrics from multiple queues #3993

Open

ctubbsii added this to the 4.0.0 milestone Jul 12, 2024

Conversation

ddanielr commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

EdColeman commented Jun 28, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ddanielr commented Jul 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ddanielr commented Jul 18, 2023

Uh oh!

ddanielr commented Jul 19, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

keith-turner Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

ddanielr Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

keith-turner Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

keith-turner commented Jul 20, 2023

Uh oh!

ddanielr commented Jul 20, 2023

Uh oh!

keith-turner commented Jul 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ddanielr commented Jun 28, 2023 •

edited

Loading