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

Post event stream #363

Merged
merged 15 commits into from
Feb 19, 2019
Merged

Post event stream #363

merged 15 commits into from
Feb 19, 2019

Conversation

vguerci
Copy link
Contributor

@vguerci vguerci commented Feb 15, 2019

Cuttle use EventSource, with unfortunately does not support POST / body. Because of that, projects with many jobs selected are breaking on these endpoints, reaching Blaze GET url limit.
This PR integrates a PostEventSource that mimics EventSource through POST polling. It is used to replace EventSource in all cases that can fails because of this GET limit.

to reproduce front end performance issues
as it appears to be much performant with many items
looks like w/o much changes, but in case: https://react-select.com/upgrade-guide
- avoid reloading jobs_definition too often
- update screen only if paused jobs changed (fixes the popup disappearing while opening it)
- force refresh on job action (prevents inconsistent ui / backend states)
as v2 requires much more changes :/ should be better though
at some point it overflows
Also, include Tests in writeClasspath for loop support
to replace eventsource (GET) with poller (POST)
@vguerci vguerci mentioned this pull request Feb 15, 2019
@vguerci
Copy link
Contributor Author

vguerci commented Feb 15, 2019

Note that this is based on #362

Copy link
Contributor

@dufrannea dufrannea left a comment

Choose a reason for hiding this comment

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

2 minor comments. General observation : Prefer using const over let in typescript for refs you don't mutate.

Copy link
Contributor Author

@vguerci vguerci left a comment

Choose a reason for hiding this comment

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

refactored couple let into const as suggested. also reintroduced the GET for calendar/focus not to break external clients (internal CuttleDependency is using this :()

in order not to break external dependencies on calendar/focus

@dufrannea dufrannea merged commit 23464af into criteo:master Feb 19, 2019
@vguerci vguerci deleted the post-event branch February 19, 2019 13:47
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.

2 participants