adding isInstantDelete API for segment deletion #8069
Conversation
...ler/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentRestletResource.java
Outdated
Show resolved
Hide resolved
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
...-controller/src/main/java/org/apache/pinot/controller/helix/core/SegmentDeletionManager.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #8069 +/- ##
=============================================
- Coverage 71.29% 14.23% -57.06%
+ Complexity 4224 81 -4143
=============================================
Files 1599 1561 -38
Lines 82957 81538 -1419
Branches 12375 12253 -122
=============================================
- Hits 59148 11611 -47537
- Misses 19815 69065 +49250
+ Partials 3994 862 -3132
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| protected void deleteSegmentsWithDelay(final String tableName, final Collection<String> segmentIds, | ||
| final long deletionDelaySeconds) { | ||
| public void deleteSegments(String tableName, Collection<String> segmentIds, boolean isInstantDeletion) { |
There was a problem hiding this comment.
Suggestion: make the delete segments api with a time argument public, and call it with 0 (or -1) to indicate "delete right away"
| deletedSegmentDestURI.toString()); | ||
| URI fileToDeleteURI = URIUtils.getUri(_dataDir, rawTableName, URIUtils.encode(segmentId)); | ||
| PinotFS pinotFS = PinotFSFactory.create(fileToDeleteURI.getScheme()); | ||
| if (isInstantDeletion) { |
There was a problem hiding this comment.
Why not take this from the config for number of days to save the segment? If the config is 0, then delete it right here. This will avoid changing the API
There was a problem hiding this comment.
i think the config is global across the entire cluster, where this API applies to a specific table/segment only on demand.
There was a problem hiding this comment.
Then maybe add a table config to override the setting, so that the behavior is the same for all segments in the table, and not dependent on how someone chooses to delete a segment?
|
@walterddr looks like you created a new issue. I am not sure about the problem you are trying to solve, so once we have that, we can come up with a solution that makes sense. Please do clarify the problem on the issue, thanks. Would appreciate if you can hold the merge until then |
| /** | ||
| * Delete a list of segments from ideal state and remove them from the local storage. | ||
| * | ||
| * @param tableNameWithType Table name with type suffix | ||
| * @param segmentNames List of names of segment to be deleted | ||
| * @param isInstanceDelete Indicate if the deleted segments will be put in retention before deletion. |
There was a problem hiding this comment.
Could you update this description a bit? What happens if it's true and what happens if it's false.
| * @return Request response | ||
| */ | ||
| public synchronized PinotResourceManagerResponse deleteSegments(String tableNameWithType, List<String> segmentNames) { | ||
| public synchronized PinotResourceManagerResponse deleteSegments(String tableNameWithType, List<String> segmentNames, | ||
| boolean isInstanceDelete) { |
There was a problem hiding this comment.
The parameter name seems inconsistent across the PR. It'd be good to unify it.
|
closing |
|
after some thought and discussion in #8078 i think this is probably the only way to do it. because once table config is deleted, there's no way to honor the table-level retention override. (since tableConfig has already gone). The only thing we can honor is whether to instantly delete (e.g. retention can only be 0 or cluster default) which essentially making this a binary override |
Support instant delete instead of asking the retention manager to remove stuff after 7 days.