Skip to content

feat(rest): add sigv4 retry configuration defaults#3063

Open
mrutunjay-kinagi wants to merge 2 commits intoapache:mainfrom
mrutunjay-kinagi:fix-3008-sigv4-retries
Open

feat(rest): add sigv4 retry configuration defaults#3063
mrutunjay-kinagi wants to merge 2 commits intoapache:mainfrom
mrutunjay-kinagi:fix-3008-sigv4-retries

Conversation

@mrutunjay-kinagi
Copy link

Rationale for this change

When REST catalog uses SigV4 signing, retries were not configured on the mounted adapter. This change adds explicit retry defaults for signed REST requests and makes retry count configurable.

Changes:

  • Added rest.sigv4.max-retries catalog property.
  • Default retries set to 10 attempts.
  • Retries configured for idempotent methods (GET, HEAD, OPTIONS) and common throttling/transient codes (429, 500, 502, 503, 504).

Are these changes tested?

Yes.

  • Added test_sigv4_adapter_default_retry_config.
  • Added test_sigv4_adapter_override_retry_config.
  • Verified with targeted pytest run.

Are there any user-facing changes?

Yes.

  • New optional REST catalog property: rest.sigv4.max-retries.
  • Improved resilience for throttling/transient HTTP failures on SigV4-enabled REST catalogs.

)
)
self._boto_session = boto3.Session(
region_name=get_first_property_value(self._properties, AWS_REGION),

Choose a reason for hiding this comment

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

Potentially this should also be aligned with the glue catalog? E.g. inclusion of profile name:

profile_name=get_first_property_value(properties, GLUE_PROFILE_NAME, AWS_PROFILE_NAME),

Copy link
Author

Choose a reason for hiding this comment

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

Good call, agreed. I have aligned this with Glue by passing client.profile-name into the SigV4 boto3.Session initialization. I also added a regression test (test_sigv4_uses_client_profile_name) to lock this behavior. This is now pushed on this PR branch.

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