Added limit to history in Supervisor#18416
Conversation
| #### Query parameters | ||
|
|
||
| * `count` (optional) | ||
| * Maximum number of history entries to return. If not specified, returns all history entries. |
There was a problem hiding this comment.
I think we might want to call out the sorting order here. No ?
| Assert.assertEquals(Collections.emptyList(), response.getEntity()); | ||
|
|
||
| // Test with negative limit | ||
| response = supervisorResource.specGetHistory(request, "id1", -1); |
There was a problem hiding this comment.
Should count <=0 be a bad request ie 400 ?
|
Thanks for taking this up @vogievetsky !! |
cryptoe
left a comment
cryptoe
left a comment
There was a problem hiding this comment.
Minor comment.
Other than that lgtm.
| public List<VersionedSupervisorSpec> getAllForId(String id, @Nullable Integer limit) throws IllegalArgumentException | ||
| { | ||
| if (limit != null && limit <= 0) { | ||
| throw new IllegalArgumentException("Limit must be greater than zero if set"); |
There was a problem hiding this comment.
The end user is setting count but the exception message received by the end user is talking about limit.
Since there can always be a variable name change, what we could do is to this check in the :
Nit: We might want to add the limit passed just to aid debugging.
|
Added validation to supervisor resource. Not sure where to put the limit in the debugging... there isn't an existing log statement |
|
Thanks for the PR. 🚀 |
3 participants
This prevents the web console from choking if there is a ton of supervisor history.
Unlike my usual console PRs I am actually modifying the Java API for
supervisor/<id>/historyto have acountparameter. I called itlimitat first but renamed it to count to match other history APIs.