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

Adds test for merging compaction metadata #4110

Closed
wants to merge 1 commit into from
Closed

Adds test for merging compaction metadata #4110

wants to merge 1 commit into from

Conversation

keith-turner
Copy link
Contributor

No description provided.

@keith-turner keith-turner mentioned this pull request Dec 21, 2023
Closed
cshannon
cshannon approved these changes Dec 22, 2023
View reviewed changes
Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

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

LGTM, do you think it's worth adding this test to main as well?

@keith-turner
Copy link
Contributor Author

LGTM, do you think it's worth adding this test to main as well?

Yeah that would be good may be able to add this to 2.1 since it has external compactions. I noticed the need for this while working on #4109, but did not want to include it in that PR so pulled it out. I will leave this PR open until I have a chance to look into moving it back.

@cshannon
Copy link
Contributor

cshannon commented Dec 22, 2023 •
edited

LGTM, do you think it's worth adding this test to main as well?

Yeah that would be good may be able to add this to 2.1 since it has external compactions. I noticed the need for this while working on #4109, but did not want to include it in that PR so pulled it out. I will leave this PR open until I have a chance to look into moving it back.

I looked at moving it back to main and it wasn't too bad, the big thing is the constructor changed for ExternalCompactionMetadata so that would have to be resolved. I didn't peak at 2.1

@ddanielr ddanielr mentioned this pull request Dec 28, 2023
Merged
@ddanielr
Copy link
Contributor

@keith-turner I made the necessary changes for adding this test to 2.1 in #4119.
The changes are pretty minor overall.

@keith-turner
Copy link
Contributor Author

@ddanielr can this be closed now?

@ddanielr
Copy link
Contributor

ddanielr commented Jan 4, 2024

@ddanielr can this be closed now?

Yes, I completed the merge of #4119 last night which put these changes in elasticity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cshannon cshannon cshannon approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@keith-turner @cshannon @ddanielr