Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Add rate limiter configs for Presto plugin #75

Merged
merged 8 commits into from
Apr 3, 2020
Merged

Conversation

lu4nm3
Copy link
Contributor

@lu4nm3 lu4nm3 commented Apr 3, 2020

TL;DR

  • Add rate limiter configs for the Presto client
  • Replace hard-coded external location with GetRawOutputPrefix

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Issue

flyteorg/flyte#240

@lu4nm3 lu4nm3 requested a review from wild-endeavor April 3, 2020 17:08
@lu4nm3 lu4nm3 changed the title Add a rate limiter and use the RawOutputPrefix in Presto plugin Add rate limiter configs for Presto plugin Apr 3, 2020
@@ -52,6 +57,10 @@ var (
Workers: 15,
LruCacheSize: 10000,
},
RateLimiterConfig: RateLimiterConfig{
Rate: 15,
Copy link
Contributor

Choose a reason for hiding this comment

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

What will this be used for? Will it gate all calls to Mozart? Or just Presto? Will it be for only creation calls? Or both creation calls and all the status checking calls in the auto-refresh cache as well? Can you add some comments about how best to calculate a good number for this config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be used for all calls going to Mozart only for Presto. Sure I can add some comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of the values themselves, I sort of took them from the awsbatch rate limiter. I'm sure we can adjust them later based on how the Presto cluster behaves

@wild-endeavor
Copy link
Contributor

Can you create and link issue as well? Or use an existing issue?

@lu4nm3
Copy link
Contributor Author

lu4nm3 commented Apr 3, 2020

Made changes from feedback and linked issue

@lu4nm3 lu4nm3 merged commit d7113a4 into master Apr 3, 2020
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
* Add rate limiter configs

* add extern storage location

* lint

* undo

* undo2

* update field description

* Update go/tasks/plugins/presto/config/config.go

Co-Authored-By: Yee Hing Tong <[email protected]>

* add comments in field description for rate limiter

Co-authored-by: Yee Hing Tong <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants