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

Limit number of events in history #410

Merged
merged 2 commits into from
Sep 5, 2019

Conversation

robfletcher
Copy link
Contributor

@robfletcher robfletcher commented Sep 4, 2019

No description provided.

@@ -31,13 +31,13 @@ class EventController(
)
fun eventHistory(
@PathVariable("name") name: ResourceName,
@RequestParam("since", defaultValue = "1970-01-01T00:00:00Z") since: Instant
@RequestParam("maxAge", defaultValue = "P3D") maxAge: Period,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to consider Pagination/InfiniteScrolling type approach here or not needed for the size of the data set ?

Copy link
Member

Choose a reason for hiding this comment

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

just to pile onto what Sairam said — ideally I expect the UI will want some kind of before / after semantic to enable paging (as opposed to page 1/2/3 etc). I don't take issue with this maxAge filter existing, provided it will peacefully coexist with the necessary filters for infinite scroll-y type paging later when 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.

Yeah, we'll possibly want it eventually. In that case I think we'd add a page=whatever param that would offset the results.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should make sure it's token based, i.e. a ULID and order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would involve adding ULIDs to the events

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(which I'm not against doing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I should drop the max age and just have a default limit? Would be simple to add a "from ULID" to that in future. Currently I'm just trying to cut down on the volume of the responses. > 1MB of YAML is not a good thing.

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 revised the PR to remove the maxAge option and just have a limit (defaulting to 10). We can easily enough add a ULID to each event and a before parameter in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @robfletcher!

@emjburns
Copy link
Contributor

emjburns commented Sep 4, 2019

@robfletcher robfletcher changed the title Optionally limit number and/or age of events in history Limit number of events in history Sep 4, 2019
@robfletcher robfletcher force-pushed the limit-resource-history branch 2 times, most recently from 1bdb206 to 54b1246 Compare September 4, 2019 23:56
@robfletcher robfletcher merged commit 3d24e44 into spinnaker:master Sep 5, 2019
@robfletcher robfletcher deleted the limit-resource-history branch September 5, 2019 14:57
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.

5 participants