-
Notifications
You must be signed in to change notification settings - Fork 464
Support configuring compactions by tablet and output path #5720
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
base: 2.1
Are you sure you want to change the base?
Conversation
Adds two methods to compaction configurer that provide the tablet and output path for the compaction. This allows different configuration to be generated based on the tablet or volume.
@@ -34,7 +36,7 @@ public interface CompactionConfigurer { | |||
/** | |||
* @since 2.1.0 | |||
*/ | |||
public interface InitParameters { | |||
interface InitParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is public API. If a user has their own implementation, does removing public
here cause some type of runtime exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think so as it is still public since its inside an interface. My IDE highlighted that these would be public w/o the keyword and that it was unnecessary so I removed them.
@@ -47,10 +49,25 @@ public interface InitParameters { | |||
/** | |||
* @since 2.1.0 | |||
*/ | |||
public interface InputParameters { | |||
interface InputParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server code provides an implementation of these interfaces and then calls the methods on the CompactionConfigurer interface passing the object created on the server side. Is it the case that these interfaces (InitParameters and InputParameters) are not part of the public api and can change like this in a patch release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are part of the public API. This change does violate semver. I made the change because this was initially added in 2.1.0 and these methods should have been added then but were missed. Also we have a few other small changes in 2.1.4 that violate semver. I can get all of these semver violations documented in the release notes if we merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm good with this as long as we document it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at the 2.1.4 draft release notes and did not see anything about semver violations, going to add a section for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all API and SPI changes in 2.1.4 and created apache/accumulo-website#464.
Adds two methods to compaction configurer that provide the tablet and output path for the compaction. This allows different configuration to be generated based on the tablet or volume.