sunyuhan1998 commented on code in PR #10791:
URL: https://github.com/apache/gravitino/pull/10791#discussion_r3136009292
##########
clients/client-python/gravitino/filesystem/gvfs_base_operations.py:
##########
@@ -644,6 +645,97 @@ def _get_actual_file_path(
def _file_system_expired(self, expire_time: int):
return expire_time <= time.time() * 1000
+ def _calculate_credential_expire_time(self, credential_expire_time_ms:
int) -> int:
+ """Calculate the local cache expiration time for a credential.
+
+ Uses the same ratio-based calculation as the filesystem cache:
+ current_time + (credential_remaining_time * ratio)
+
+ :param credential_expire_time_ms: The credential's expiry time in
epoch ms
+ :return: The local cache expiration time in epoch ms
+ """
+ if credential_expire_time_ms <= 0:
+ return TIME_WITHOUT_EXPIRATION
+
+ ratio = float(
+ self._options.get(
+ GVFSConfig.GVFS_FILESYSTEM_CREDENTIAL_EXPIRED_TIME_RATIO,
+ GVFSConfig.DEFAULT_CREDENTIAL_EXPIRED_TIME_RATIO,
+ )
+ if self._options
+ else GVFSConfig.DEFAULT_CREDENTIAL_EXPIRED_TIME_RATIO
+ )
+ now_ms = time.time() * 1000
+ return int(now_ms + (credential_expire_time_ms - now_ms) * ratio)
+
+ def _get_credentials_with_cache(
+ self,
+ fileset_ident: NameIdentifier,
+ fileset: Fileset,
+ target_location_name: str,
+ ) -> Optional[List[Credential]]:
+ """Get credentials with client-side caching to avoid redundant REST
calls.
+
+ Implements lazy refresh: returns cached credentials if not expired,
+ otherwise fetches new credentials from the server.
+
+ :param fileset_ident: The fileset identifier
+ :param fileset: The fileset object
+ :param target_location_name: The resolved location name
+ :return: List of credentials, or None if credential vending is disabled
+ """
+ if not self._enable_credential_vending:
+ return None
+
+ cache_key = (fileset_ident, target_location_name)
+
+ # Fast path: read lock, check cache
+ read_lock = self._credential_cache_lock.gen_rlock()
+ try:
+ read_lock.acquire()
+ cache_value = self._credential_cache.get(cache_key)
+ if cache_value is not None:
+ expire_time, cached_credentials = cache_value
+ if not self._file_system_expired(expire_time):
+ return cached_credentials
+ finally:
+ read_lock.release()
+
+ # Slow path: write lock, double-check, then fetch
+ write_lock = self._credential_cache_lock.gen_wlock()
+ try:
+ write_lock.acquire()
+ # Double-check after acquiring write lock
+ cache_value = self._credential_cache.get(cache_key)
+ if cache_value is not None:
+ expire_time, cached_credentials = cache_value
+ if not self._file_system_expired(expire_time):
+ return cached_credentials
+
+ # Fetch fresh credentials from server
+ fresh_credentials = fileset.support_credentials().get_credentials()
+
+ if fresh_credentials:
+ expirable = [
+ c.expire_time_in_ms()
+ for c in fresh_credentials
+ if c.expire_time_in_ms() > 0
+ ]
Review Comment:
Good catch. Added
`test_mixed_credentials_cache_expiry_based_on_expiring_one` to cover the mixed
credential scenario (never-expire + expiring), verifying that cache expiry is
driven by the expiring credential.
##########
clients/client-python/tests/unittests/test_gvfs_credential_cache.py:
##########
@@ -0,0 +1,319 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint:
disable=protected-access,import-outside-toplevel,no-value-for-parameter,broad-exception-caught,unsubscriptable-object
+import sys
+import threading
+import time
+import unittest
+from unittest.mock import MagicMock
+
+from gravitino.api.credential.s3_token_credential import S3TokenCredential
+from gravitino.filesystem.gvfs_config import GVFSConfig
+from gravitino.name_identifier import NameIdentifier
+
+
+def _create_mock_credential(expire_time_in_ms):
+ """Create a mock S3TokenCredential with the given expire time."""
+ return S3TokenCredential(
+ credential_info={
+ S3TokenCredential._SESSION_ACCESS_KEY_ID: "access_id",
+ S3TokenCredential._SESSION_SECRET_ACCESS_KEY: "secret_key",
+ S3TokenCredential._SESSION_TOKEN: "session_token",
+ },
+ expire_time_in_ms=expire_time_in_ms,
+ )
+
+
+def _create_mock_fileset(credentials_list):
+ """Create a mock fileset that returns the given credentials."""
+ from gravitino.client.generic_fileset import GenericFileset
+
+ mock_support = MagicMock()
+ mock_support.get_credentials.return_value = credentials_list
+ mock_fileset = MagicMock(spec=GenericFileset)
+ mock_fileset.support_credentials.return_value = mock_support
+ return mock_fileset
+
+
+class _ConcreteGVFSOperations:
+ """Minimal concrete subclass to test BaseGVFSOperations credential cache
logic."""
+
+ pass
+
+
Review Comment:
You'\''re right, removed the unused `_ConcreteGVFSOperations` class.
--
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]
