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

Allow periodic tasks to run with properties via the POST API #14915

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

9aman
Copy link
Contributor

@9aman 9aman commented Jan 24, 2025

POST API:

  • Current perodic task run API does not take periodic task properties as input.
  • Introduce a POST API to allow passing in periodic task properties as well.

Running segment level validaitons in RealtimeSegmentValidationManager

  • The RealtimeSegmentValidationManager performs segment level validations to upload segments that have missing url.
  • Segment level validation run at a specific frequency and cannot be triggered using the controller periodic task API
  • The addition of above APi allows passage of task properties.
  • Additional field is checked in the task properties to run segment level validations even when time constraints are not met.

Local Testing:

The segment level validations run even when the time constraints are not met.
trigger_validations

Returning_true_for_validations
Running_validations

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 63.71%. Comparing base (59551e4) to head (f3a3ad5).
Report is 1623 commits behind head on master.

Files with missing lines Patch % Lines
...es/PinotControllerPeriodicTaskRestletResource.java 0.00% 13 Missing ⚠️
...r/validation/RealtimeSegmentValidationManager.java 0.00% 10 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #14915      +/-   ##
============================================
+ Coverage     61.75%   63.71%   +1.96%     
- Complexity      207     1470    +1263     
============================================
  Files          2436     2708     +272     
  Lines        133233   151565   +18332     
  Branches      20636    23404    +2768     
============================================
+ Hits          82274    96567   +14293     
- Misses        44911    47745    +2834     
- Partials       6048     7253    +1205     
Flag Coverage Δ
custom-integration1 100.00% <ø> (+99.99%) ⬆️
integration 100.00% <ø> (+99.99%) ⬆️
integration1 100.00% <ø> (+99.99%) ⬆️
integration2 0.00% <ø> (ø)
java-11 63.67% <0.00%> (+1.96%) ⬆️
java-21 63.61% <0.00%> (+1.98%) ⬆️
skip-bytebuffers-false 63.69% <0.00%> (+1.94%) ⬆️
skip-bytebuffers-true 63.58% <0.00%> (+35.86%) ⬆️
temurin 63.71% <0.00%> (+1.96%) ⬆️
unittests 63.70% <0.00%> (+1.96%) ⬆️
unittests1 56.29% <ø> (+9.40%) ⬆️
unittests2 34.01% <0.00%> (+6.28%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.EXECUTE_TASK)
@ApiOperation(value = "Run periodic task against table with custom properties. If table name is missing, task will "
+ "run against all tables.")
public Response runPeriodicTaskWithProperties(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why a new method? Are we not able to just add param to existing method without breaking backward compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not want to keep it as a query param for the GET API.
Passing Map/ API param in a GET request also looks a bit weird (against HTTP standards). That's why introduced POST with API params.

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.

3 participants