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.

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

adding instant deletion option for segment deletion #8122

Merged
merged 1 commit into from
Feb 20, 2022

Conversation

walterddr
Copy link
Contributor

@walterddr walterddr commented Feb 3, 2022

per discussion in #8072, #8078 and #8176

We will add deletion with retention period overwrite for segment deletion.

@@ -571,12 +571,15 @@ public SuccessResponse reloadAllSegmentsDeprecated2(
@ApiOperation(value = "Delete a segment", notes = "Delete a segment")
public SuccessResponse deleteSegment(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "Name of the segment", required = true) @PathParam("segmentName") @Encoded String segmentName) {
@ApiParam(value = "Name of the segment", required = true) @PathParam("segmentName") @Encoded String segmentName,
@ApiParam(value = "Whether to delete the segment instantly or move to deleted_segment prefix and let "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
@ApiParam(value = "Whether to delete the segment instantly or move to deleted_segment prefix and let "
@ApiParam(value = "Whether to delete the segment without backup"

@ApiParam(value = "Name of the segment", required = true) @PathParam("segmentName") @Encoded String segmentName) {
@ApiParam(value = "Name of the segment", required = true) @PathParam("segmentName") @Encoded String segmentName,
@ApiParam(value = "Whether to delete the segment instantly or move to deleted_segment prefix and let "
+ "RetentionManager handle the actual file deletion") @QueryParam("instantDelete") @DefaultValue("false")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+ "RetentionManager handle the actual file deletion") @QueryParam("instantDelete") @DefaultValue("false")
) @QueryParam("deleteWithoutBackup") @DefaultValue("false")

Copy link
Contributor Author

@walterddr walterddr Feb 17, 2022

Choose a reason for hiding this comment

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

with #8176 we will be able to change the API to accept retention period, which was what you originally suggested

Suggested change
+ "RetentionManager handle the actual file deletion") @QueryParam("instantDelete") @DefaultValue("false")
+ @ApiParam(value = "segment delete retention period, (for example 12h, 3d); setting it to 0d will instant delete the segments") @QueryParam("deleteRetention") @DefaultValue("7d") deleteRetentionPeriod

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #8122 (0949a1b) into master (287f552) will increase coverage by 0.06%.
The diff coverage is 61.53%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8122      +/-   ##
============================================
+ Coverage     71.01%   71.07%   +0.06%     
  Complexity     4320     4320              
============================================
  Files          1626     1626              
  Lines         85067    85072       +5     
  Branches      12799    12800       +1     
============================================
+ Hits          60408    60468      +60     
+ Misses        20505    20449      -56     
- Partials       4154     4155       +1     
Flag Coverage Δ
integration1 28.80% <53.84%> (+0.04%) ⬆️
integration2 27.53% <0.00%> (+0.09%) ⬆️
unittests1 67.35% <ø> (-0.01%) ⬇️
unittests2 14.13% <46.15%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ler/api/resources/PinotSegmentRestletResource.java 28.50% <50.00%> (ø)
...ntroller/helix/core/PinotHelixResourceManager.java 65.96% <50.00%> (-0.05%) ⬇️
.../controller/helix/core/SegmentDeletionManager.java 75.62% <100.00%> (-1.11%) ⬇️
...er/validation/BrokerResourceValidationManager.java 81.25% <0.00%> (-18.75%) ⬇️
...ache/pinot/core/operator/docidsets/OrDocIdSet.java 86.36% <0.00%> (-11.37%) ⬇️
.../helix/core/minion/MinionInstancesCleanupTask.java 77.27% <0.00%> (-4.55%) ⬇️
...core/startree/operator/StarTreeFilterOperator.java 83.91% <0.00%> (-3.50%) ⬇️
...rg/apache/pinot/broker/routing/RoutingManager.java 87.71% <0.00%> (-1.76%) ⬇️
...nMaxValueBasedSelectionOrderByCombineOperator.java 71.96% <0.00%> (-0.76%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 287f552...0949a1b. Read the comment docs.

@walterddr walterddr changed the title adding isInstantDelete API for segment deletion adding instant deletion option for segment deletion Feb 17, 2022
@Jackie-Jiang Jackie-Jiang merged commit 4f17ede into apache:master Feb 20, 2022
xiangfu0 pushed a commit to xiangfu0/pinot that referenced this pull request Feb 23, 2022
Add deletion with retention period overwrite for segment deletion
@walterddr walterddr deleted the instant_delete_segment_api branch December 6, 2023 16:21
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.

4 participants