Skip to content

Boto Glue standard retry policy with configuration #1307

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Nov 20, 2024
Merged

Boto Glue standard retry policy with configuration #1307

merged 4 commits into from
Nov 20, 2024

Conversation

mark-major
Copy link
Contributor

@mark-major mark-major commented Nov 8, 2024 •
edited
Loading

Configurable retry mechanism for the Glue catalog Boto client.

  • Setting the retry mode to standard
  • Introducing a new configuration glue.max-retries with default value set to 10
  • Adding the new configuration key to the documentation

Closes #1294

@mark-major
boto glue standard retry policy
d589fde 
configurable max retry
@mark-major mark-major marked this pull request as ready for review November 8, 2024 18:10
@mark-major mark-major mentioned this pull request Nov 8, 2024
Closed
Fokko
Fokko reviewed Nov 8, 2024
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks @mark-major for raising this PR! I left some suggestions, LMKWYT!

pyiceberg/catalog/glue.py Outdated
"max_attempts": get_first_property_value(properties, GLUE_MAX_RETRIES)
if get_first_property_value(properties, GLUE_MAX_RETRIES)
else MAX_RETRIES,
"mode": "standard",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also make the mode configurable? :)

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 would rather not have that as an option. Standard mode is a more mature implementation compared to the legacy one.Adaptive mode is still considered 'experimental' which is not something I'd use in production.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to making it configurable just to future-proof.
Still set standard as the default, but can be overridden if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll make the mode configurable

Copy link
Contributor

Choose a reason for hiding this comment

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

@mark-major We're almost there, are you interested in pushing this over the finish line?

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 have pushed the changes, sorry for taking this long...

mark-major and others added 2 commits November 9, 2024 00:35
@mark-major @Fokko
Update configuration.md
c9bfeeb 
Co-authored-by: Fokko Driesprong <[email protected]>
@mark-major @Fokko
Update glue.py
cd21f97 
Co-authored-by: Fokko Driesprong <[email protected]>
kevinjqliu
kevinjqliu reviewed Nov 9, 2024
View reviewed changes
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

A few nit comments. Curious about the general direction we're taking for configuring the retry mechanism for catalogs. I'm leaning towards a standardized retry config for all catalogs, similar to the commit.retry table configs https://iceberg.apache.org/docs/1.5.2/configuration/#table-behavior-properties

Curious to hear others' thoughts

mkdocs/docs/configuration.md
| glue.access-key-id | admin | Configure the static access key id used to access the Glue Catalog |
| glue.secret-access-key | password | Configure the static secret access key used to access the Glue Catalog |
| glue.session-token | AQoDYXdzEJr... | Configure the static session token used to access the Glue Catalog |
| glue.max-retries | 10 | Configure the maximum number of retries for the Glue service calls |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, should we add a retry property for each catalog? Or just use the default value in the table retry property?`

commit.retry.num-retries | 4 | Number of times to retry a commit before failing

https://iceberg.apache.org/docs/1.5.2/configuration/#table-behavior-properties

pyiceberg/catalog/glue.py Outdated
"max_attempts": get_first_property_value(properties, GLUE_MAX_RETRIES)
if get_first_property_value(properties, GLUE_MAX_RETRIES)
else MAX_RETRIES,
"mode": "standard",
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to making it configurable just to future-proof.
Still set standard as the default, but can be overridden if necessary

Fokko
Fokko approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this 👍

@Fokko Fokko merged commit 12e87a4 into apache:main Nov 20, 2024
7 checks passed
This was referenced Nov 25, 2024
Merged
Merged
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
@mark-major @Fokko @sungwy
Boto Glue standard retry policy with configuration (apache#1307)
705c5ae 
* boto glue standard retry policy
configurable max retry

* Update configuration.md

Co-authored-by: Fokko Driesprong <[email protected]>

* Update glue.py

Co-authored-by: Fokko Driesprong <[email protected]>

* boto glue retry mode configurable

---------

Co-authored-by: Fokko Driesprong <[email protected]>
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
@mark-major @Fokko @sungwy
Boto Glue standard retry policy with configuration (apache#1307)
e1f73ec 
* boto glue standard retry policy
configurable max retry

* Update configuration.md

Co-authored-by: Fokko Driesprong <[email protected]>

* Update glue.py

Co-authored-by: Fokko Driesprong <[email protected]>

* boto glue retry mode configurable

---------

Co-authored-by: Fokko Driesprong <[email protected]>
JE-Chen pushed a commit to JE-Chen/iceberg-python that referenced this pull request Dec 23, 2024
@mark-major @Fokko @JE-Chen
Boto Glue standard retry policy with configuration (apache#1307)
fce785d 
* boto glue standard retry policy
configurable max retry

* Update configuration.md

Co-authored-by: Fokko Driesprong <[email protected]>

* Update glue.py

Co-authored-by: Fokko Driesprong <[email protected]>

* boto glue retry mode configurable

---------

Co-authored-by: Fokko Driesprong <[email protected]>
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 24, 2024
@mark-major @Fokko @sungwy
Boto Glue standard retry policy with configuration (apache#1307)
882edec 
* boto glue standard retry policy
configurable max retry

* Update configuration.md

Co-authored-by: Fokko Driesprong <[email protected]>

* Update glue.py

Co-authored-by: Fokko Driesprong <[email protected]>

* boto glue retry mode configurable

---------

Co-authored-by: Fokko Driesprong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinjqliu kevinjqliu kevinjqliu left review comments

@Fokko Fokko Fokko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adaptive retry for Glue Catalog calls to fix Glue throttling
3 participants
@mark-major @Fokko @kevinjqliu