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.

Sign up for GitHub

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

Jump to bottom

Add compaction IT that verifies queues are cleared when tablets no longer need to compact #4466

Merged
merged 10 commits into from May 15, 2024
Merged

Add compaction IT that verifies queues are cleared when tablets no longer need to compact #4466

merged 10 commits into from May 15, 2024

Conversation

DomGarguilo
Copy link
Member

Addresses #4399

@DomGarguilo DomGarguilo self-assigned this Apr 17, 2024
@DomGarguilo
Copy link
Member Author

This test is failing at the moment but I wanted to put up what I have so far because, to me, it seems like it should be testing things correctly. The issue is probably with my assumptions in setting the test but may be correctly pointing to a bug.

The test essentially sets things up so that there are compactions queued (with no compactors running that would carry out the compactions) and then the compaction settings are changed so that compactions no longer need to happen. It is expected that the queue would be emptied but I do not see that happening. Here is a portion of the logs while the test is waiting for the queue to be emptied:

2024-04-17T10:45:10,652 55 [compaction.CompactionPriorityQueueMetricsIT] INFO : Queue size: 6 Jobs Queued: 6 Jobs Dequeued: 0 Jobs Rejected: 96
2024-04-17T10:45:14,153 55 [compaction.CompactionPriorityQueueMetricsIT] INFO : Queue size: 6 Jobs Queued: 6 Jobs Dequeued: 0 Jobs Rejected: 99
2024-04-17T10:45:17,653 55 [compaction.CompactionPriorityQueueMetricsIT] INFO : Queue size: 6 Jobs Queued: 6 Jobs Dequeued: 0 Jobs Rejected: 102
2024-04-17T10:45:21,153 55 [compaction.CompactionPriorityQueueMetricsIT] INFO : Queue size: 6 Jobs Queued: 6 Jobs Dequeued: 0 Jobs Rejected: 102
2024-04-17T10:45:24,654 55 [compaction.CompactionPriorityQueueMetricsIT] INFO : Queue size: 6 Jobs Queued: 6 Jobs Dequeued: 0 Jobs Rejected: 105
2024-04-17T10:45:28,154 55 [compaction.CompactionPriorityQueueMetricsIT] INFO : Queue size: 6 Jobs Queued: 6 Jobs Dequeued: 0 Jobs Rejected: 108

These are the last logs before the test times out. Note the queue sizes and the Jobs rejected.

@DomGarguilo DomGarguilo marked this pull request as ready for review April 18, 2024 17:21
kevinrr888
kevinrr888 reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@kevinrr888 kevinrr888 left a comment

Choose a reason for hiding this comment

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

Overall, looks good and verified test passes

test/src/main/java/org/apache/accumulo/test/compaction/CompactionPriorityQueueMetricsIT.java Outdated Show resolved Hide resolved
test/src/main/java/org/apache/accumulo/test/compaction/CompactionPriorityQueueMetricsIT.java Outdated Show resolved Hide resolved
test/src/main/java/org/apache/accumulo/test/compaction/CompactionPriorityQueueMetricsIT.java Outdated Show resolved Hide resolved
test/src/main/java/org/apache/accumulo/test/compaction/CompactionPriorityQueueMetricsIT.java Outdated Show resolved Hide resolved
@DomGarguilo DomGarguilo linked an issue May 1, 2024 that may be closed by this pull request
Add compaction IT that verifies queues are cleared when tablets no longer need to compact. #4399
Closed
keith-turner
keith-turner reviewed May 9, 2024
View reviewed changes
test/src/main/java/org/apache/accumulo/test/compaction/CompactionPriorityQueueMetricsIT.java Outdated
return false;
}, 60_000, sleepMillis, "did not see Q1 metrics");

Wait.waitFor(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this wait loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized I pulled in a lot of unneeded stuff from the other test case while developing the changes for this PR. In 9a63f3e I removed a lot so that its easier to track whats going on and only kept whats necessary to test what we are trying to test here.

@DomGarguilo DomGarguilo merged commit ff951d7 into apache:elasticity May 15, 2024
8 checks passed
@DomGarguilo DomGarguilo deleted the verifyQueuesCleared branch May 15, 2024 13:32
@DomGarguilo DomGarguilo mentioned this pull request May 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith-turner keith-turner keith-turner approved these changes

@kevinrr888 kevinrr888 kevinrr888 approved these changes

Assignees

@DomGarguilo DomGarguilo

Labels
None yet
Projects
Accumulo Elasticity
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add compaction IT that verifies queues are cleared when tablets no longer need to compact.
3 participants
@DomGarguilo @keith-turner @kevinrr888