kevinjqliu commented on code in PR #3063: URL: https://github.com/apache/iceberg-python/pull/3063#discussion_r2900120580
########## pyiceberg/catalog/rest/__init__.py: ########## @@ -228,6 +236,8 @@ class IdentifierKind(Enum): SIGV4 = "rest.sigv4-enabled" SIGV4_REGION = "rest.signing-region" SIGV4_SERVICE = "rest.signing-name" +SIGV4_MAX_RETRIES = "rest.sigv4.max-retries" +SIGV4_DEFAULT_MAX_RETRIES = 10 Review Comment: nit: could you use _DEFAULT to match the convention https://grep.app/search?f.repo=apache%2Ficeberg-python&f.repo.pattern=apache%2Ficeberg&case=true&q=_DEFAULT+%3D ```suggestion SIGV4_MAX_RETRIES_DEFAULT = 10 ``` ########## pyiceberg/catalog/rest/__init__.py: ########## @@ -688,9 +698,11 @@ def _init_sigv4(self, session: Session) -> None: class SigV4Adapter(HTTPAdapter): def __init__(self, **properties: str): - super().__init__() self._properties = properties + max_retries = property_as_int(self._properties, SIGV4_MAX_RETRIES, SIGV4_DEFAULT_MAX_RETRIES) Review Comment: ```suggestion max_retries = property_as_int(self._properties, SIGV4_MAX_RETRIES, SIGV4_MAX_RETRIES_DEFAULT) ``` ########## tests/catalog/test_rest.py: ########## @@ -33,6 +33,8 @@ DEFAULT_ENDPOINTS, EMPTY_BODY_SHA256, OAUTH2_SERVER_URI, + SIGV4_DEFAULT_MAX_RETRIES, Review Comment: ```suggestion SIGV4_MAX_RETRIES_DEFAULT, ``` ########## tests/catalog/test_rest.py: ########## @@ -529,6 +531,66 @@ def test_sigv4_sign_request_with_body(rest_mock: Mocker) -> None: assert prepared.headers.get("x-amz-content-sha256") != EMPTY_BODY_SHA256 +def test_sigv4_adapter_default_retry_config(rest_mock: Mocker) -> None: + catalog = RestCatalog( + "rest", + **{ + "uri": TEST_URI, + "token": TEST_TOKEN, + "rest.sigv4-enabled": "true", + "rest.signing-region": "us-west-2", + "client.access-key-id": "id", + "client.secret-access-key": "secret", + }, + ) + + adapter = catalog._session.adapters[catalog.uri] + assert isinstance(adapter, HTTPAdapter) + assert adapter.max_retries.total == SIGV4_DEFAULT_MAX_RETRIES Review Comment: ```suggestion assert adapter.max_retries.total == SIGV4_MAX_RETRIES_DEFAULT ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
