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

Don't throttle requests by server if RequestScheduler.throttleRequests is false #8385

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

lilleyse
Copy link
Contributor

Requests that are throttled by server should not be throttled if RequestScheduler.throttleRequests is false. Not sure if this has come up in practice since RequestScheduler.throttleRequests is true by default and is not visible to the public API.

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@kring
Copy link
Member

kring commented Nov 12, 2019

@lilleyse I did this purposely in #7061. Some explanation of why here: #7061 (comment)

In short, terrain/imagery wants to do throttling by server (drop requests when too many are already in flight), but it does its own prioritization, so the throttleRequests code in RequestScheduler is expensive and counter-productive.

@lilleyse
Copy link
Contributor Author

@kring This isn't changing that behavior. The actual throttling mechanism when RequestScheduler.throttleRequests is true has not been changed. This fix is so that if someone sets RequestScheduler.throttleRequests = false; terrain/imagery requests don't get throttled.

Some more background - earlier I was testing the WMS sandcastle demo with RequestScheduler.throttleRequests = false; and was confused that requests were still being throttled. My assumption was that throttleRequests=false was a global override until I looked at the code again.

@kring
Copy link
Member

kring commented Nov 12, 2019

Oh, right, I see. You're checking the throttleRequests property on RequestScheduler, not on the request itself. I misread that.

@cesium-concierge
Copy link

Thanks again for your contribution @lilleyse!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @lilleyse!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 29, 2020

@lilleyse @kring what's the status of this PR?

@lilleyse
Copy link
Contributor Author

No more changes from me, just needs a review (which should be quick)

@kring kring merged commit db3f21e into master Jan 30, 2020
@kring kring deleted the throttle-requests-disabled branch January 30, 2020 00:14
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.

4 participants