Prevent 500 error for non-existent tasktype in /tasks/{taskType}/tasks API

[soumitra-st]

Before fix

Curl output

% curl -k "http://localhost:9000/tasks/REFRESH/tasks" -w "\n%{http_code}\n"
{"code":500,"error":"Task queue: TaskQueue_REFRESH for task type: REFRESH does not exist"}
500

Log in the controller

2024/07/03 21:07:02.350 ERROR [WebApplicationExceptionMapper] [grizzly-http-server-0] Server error:
java.lang.IllegalArgumentException: Task queue: TaskQueue_REFRESH for task type: REFRESH does not exist
	at org.apache.pinot.shaded.com.google.common.base.Preconditions.checkArgument(Preconditions.java:448)
	at org.apache.pinot.controller.helix.core.minion.PinotHelixTaskResourceManager.getTasks(PinotHelixTaskResourceManager.java:320)
	at org.apache.pinot.controller.api.resources.PinotTaskRestletResource.getTasks(PinotTaskRestletResource.java:194)

After fix

Curl output

% curl -k "http://localhost:9000/tasks/REFRESH/tasks" -w "\n%{http_code}\n"
[]
200

[codecov-commenter]

Codecov Report

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

Project coverage is 62.03%. Comparing base (59551e4) to head (808e2ed). Report is 807 commits behind head on master.

Files	Patch %	Lines
...roller/api/resources/PinotTaskRestletResource.java	0.00%	4 Missing :warning:
...lix/core/minion/PinotHelixTaskResourceManager.java	0.00%	4 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13537      +/-   ##
============================================
+ Coverage     61.75%   62.03%   +0.28%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2554     +118     
  Lines        133233   140617    +7384     
  Branches      20636    21876    +1240     
============================================
+ Hits          82274    87234    +4960     
- Misses        44911    46742    +1831     
- Partials       6048     6641     +593     

Flag	Coverage Δ
custom-integration1	<0.01% <0.00%> (-0.01%)	:arrow_down:
integration	<0.01% <0.00%> (-0.01%)	:arrow_down:
integration1	<0.01% <0.00%> (-0.01%)	:arrow_down:
integration2	0.00% <0.00%> (ø)
java-11	61.97% <0.00%> (+0.26%)	:arrow_up:
java-21	61.89% <0.00%> (+0.26%)	:arrow_up:
skip-bytebuffers-false	62.02% <0.00%> (+0.28%)	:arrow_up:
skip-bytebuffers-true	61.85% <0.00%> (+34.13%)	:arrow_up:
temurin	62.03% <0.00%> (+0.28%)	:arrow_up:
unittests	62.03% <0.00%> (+0.28%)	:arrow_up:
unittests1	46.44% <ø> (-0.45%)	:arrow_down:
unittests2	27.81% <0.00%> (+0.08%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[mayankshriv]

Shouldn't the response indicate that the task was nonexistent? Silently returning 200 can confuse the user. Similarly for controller log, we should log something to indicate the same (instead of exception, or no log).

% curl -k "http://localhost:9000/tasks/REFRESH/tasks" -w "\n%{http_code}\n"
[]
200

[soumitra-st]

+1. We need to return error to the user because the task doesn't exist

We return 200 for non-existent table, so followed the same protocol here.

% curl -k https://.../tables/blahsk -w "\nHTTP Code: %{http_code}\n"
{}
HTTP Code: 200

Let me change the return to 404 in this case though.