Skip to content

adding API for retention period on table rest resource#9148

Merged
walterddr merged 2 commits intoapache:masterfrom
walterddr:add_table_delete_retention_api
Aug 4, 2022
Merged

adding API for retention period on table rest resource#9148
walterddr merged 2 commits intoapache:masterfrom
walterddr:add_table_delete_retention_api

Conversation

@walterddr
Copy link
Contributor

@walterddr walterddr commented Aug 2, 2022

This fixes #9115

@walterddr walterddr marked this pull request as ready for review August 2, 2022 22:54
@ApiParam(value = "Name of the table to delete", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "realtime|offline") @QueryParam("type") String tableTypeStr) {
@ApiParam(value = "realtime|offline") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "Retention period for the table segments (e.g. 12h, 3d); Using 0d or -1d will instantly "
Copy link
Contributor

Choose a reason for hiding this comment

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

can you clarify if this retention period on the server or on the deep store?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this retention period in the query override any retention setting at the table or controller config level ? Best to clarify the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea. see my suggested change below


@Nullable
private static Long getRetentionMsFromTableConfig(@Nullable TableConfig tableConfig) {
public static Long getRetentionMsFromTableConfig(@Nullable TableConfig tableConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes the util was only used within SegmentDeletionManager but now we need to expose it to not just segment but table utils.

@ApiParam(value = "Name of the table to delete", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "realtime|offline") @QueryParam("type") String tableTypeStr) {
@ApiParam(value = "realtime|offline") @QueryParam("type") String tableTypeStr,
@ApiParam(value = "Retention period for the table segments (e.g. 12h, 3d); Using 0d or -1d will instantly "
Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea. see my suggested change below

Comment on lines +421 to +422
@ApiParam(value = "Retention period for the table segments (e.g. 12h, 3d); Using 0d or -1d will instantly "
+ "delete segments without retention") @QueryParam("retention") String retentionPeriod) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
@ApiParam(value = "Retention period for the table segments (e.g. 12h, 3d); Using 0d or -1d will instantly "
+ "delete segments without retention") @QueryParam("retention") String retentionPeriod) {
@ApiParam(value = "Retention period for the table segments (e.g. 12h, 3d); "
+ " If not set, the retention period will default to cluster setting; Using 0d or -1d will instantly "
+ "delete segments without retention.") @QueryParam("retention") String retentionPeriod) {

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the retention period specified in the table? :)
Perhaps it's simpler to use a boolean flag for whether they want instant delete or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Retention period on table will not be honored since the table config will be deleted first.

@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2022

Codecov Report

Merging #9148 (1e28f7a) into master (55db06f) will decrease coverage by 44.93%.
The diff coverage is 70.07%.

❗ Current head 1e28f7a differs from pull request most recent head cc3502b. Consider uploading reports for the commit cc3502b to get more accurate results

@@              Coverage Diff              @@
##             master    #9148       +/-   ##
=============================================
- Coverage     69.91%   24.97%   -44.94%     
+ Complexity     4993       53     -4940     
=============================================
  Files          1844     1836        -8     
  Lines         97659    97553      -106     
  Branches      14721    14710       -11     
=============================================
- Hits          68280    24367    -43913     
- Misses        24610    70802    +46192     
+ Partials       4769     2384     -2385     
Flag Coverage Δ
integration1 ?
integration2 24.97% <70.07%> (+0.20%) ⬆️
unittests1 ?
unittests2 ?

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

Impacted Files Coverage Δ
.../controller/helix/core/SegmentDeletionManager.java 23.12% <ø> (-51.88%) ⬇️
...segment/local/data/manager/SegmentDataManager.java 0.00% <0.00%> (-83.34%) ⬇️
...va/org/apache/pinot/spi/utils/CommonConstants.java 0.00% <0.00%> (-28.58%) ⬇️
...spi/utils/builder/ControllerRequestURLBuilder.java 0.00% <0.00%> (ø)
...oller/api/resources/PinotTableRestletResource.java 27.72% <20.00%> (-36.82%) ⬇️
...ver/api/resources/ControllerJobStatusResource.java 60.00% <60.00%> (ø)
...ntroller/helix/core/PinotHelixResourceManager.java 31.87% <61.29%> (-36.47%) ⬇️
...ler/api/resources/PinotSegmentRestletResource.java 31.19% <70.10%> (+4.34%) ⬆️
.../pinot/common/function/scalar/StringFunctions.java 38.23% <96.15%> (-32.82%) ⬇️
...pinot/broker/broker/BrokerAdminApiApplication.java 89.58% <100.00%> (+0.22%) ⬆️
... and 1364 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@walterddr walterddr merged commit 2eadde4 into apache:master Aug 4, 2022
@walterddr walterddr deleted the add_table_delete_retention_api branch December 6, 2023 16:16
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.

Add a retention override for delete table REST API

4 participants