Skip to content

Conversation

@kevinrr888
Copy link
Member

@kevinrr888 kevinrr888 commented Dec 27, 2023

Closes #3937
Changes:

  • Added getSelectedFiles() method to CompactionConfigurer.InputParameters
  • Added method functionality in the two implementations of this: ShellCompactCommandConfigurerTest and CompactableUtils
  • Tested functionality in new IT: testGetSelectedFilesForCompaction() in CompactionIT
  • Added new method getCompressionType() to PrintBCInfo (used in the test)
  • Changed several method signatures to pass around necessary info for getSelectedFiles() implementations
  • Renamed 'files' variable to 'inputFiles' in CompactableUtils and CompactableImpl for a more clear distinction between inputFiles and the (new) selectedFiles.
  • Moved 'fsin' in PrintBCInfo to try-with-resources block

Potentially still left TODO:

  • Add this functionality for external compactions?
  • Move CompressionTypeConfigurer to it's own class?

Changes
- Added getSelectedFiles() method to CompactionConfigurer.InputParameters
- Added method functionality in the two implementations of this: ShellCompactCommandConfigurerTest and CompactableUtils
- Tested functionality in new IT: testGetSelectedFilesForCompaction() in CompactionIT
- Added new method getCompressionType() to PrintBCInfo (used in the test)
- Changed several method signatures to pass around necessary info for getSelectedFiles() implementations
- Renamed 'files' variable to 'inputFiles' in CompactableUtils and CompactableImpl for a more clear distinction between inputFiles and the (new) selectedFiles.
Potentially still left TODO:
- Add this functionality for external compactions?
- Move CompressionTypeConfigurer to it's own class?
* <ul>
* <li>There is no selected set of files so the empty set is returned.</li>
* </ul>
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a new API method so needs a since tag.

Suggested change
*/
*
* @since 3.1
*/

@keith-turner
Copy link
Contributor

Add this functionality for external compactions?

If you added the new method to all impls of CompactionConfigurer then I would think this case is already covered. However I did not actually follow the code to be sure.

@kevinrr888
Copy link
Member Author

Add this functionality for external compactions?

If you added the new method to all impls of CompactionConfigurer then I would think this case is already covered. However I did not actually follow the code to be sure.

Okay thanks! I did add the new method to the implementations of CompactionConfigurer.

@keith-turner keith-turner merged commit d02340a into apache:main Jan 4, 2024
@EdColeman
Copy link
Contributor

@kevinrr888 - there are some todo's in the description that seem like they might be worth while for someone to pursue. Have those been captured in an issue?

@kevinrr888
Copy link
Member Author

@EdColeman thanks for pointing these out. The external compactions todo has been completed in this issue (I originally wasn't sure if my changes also worked for external compactions, but they do). The todo about moving the class probably isn't needed and is probably better suited for where it is now since it's currently only used in CompactionIT.

@ctubbsii ctubbsii added this to the 3.1.0 milestone Jul 12, 2024
@kevinrr888 kevinrr888 deleted the feature-3937 branch November 1, 2024 15:09
@ctubbsii ctubbsii modified the milestones: 3.1.0, 4.0.0 Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide indication of intermediate compactions to CompactionConfigurer.

5 participants