sunyuhan1998 commented on code in PR #10791:
URL: https://github.com/apache/gravitino/pull/10791#discussion_r3136000530
##########
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)
Review Comment:
Thanks for the suggestion. `credential_expiration_ratio` is an existing
config key, and the original implementation in
`StorageHandler._get_expire_time_by_ratio` (`gvfs_storage_handler.py:187`) also
does not validate its range. Consistency with the existing code takes
precedence over adding validation in just this one new method. If range
validation is needed, it should be addressed as a separate improvement applied
uniformly across all usages of this config.
##########
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)
Review Comment:
Thanks for pointing this out. The two methods do share similar logic, but
they belong to different classes (`BaseGVFSOperations` and `StorageHandler`)
with distinct use cases (filesystem cache vs. credential cache). Extracting a
shared helper would require restructuring module dependencies, which is beyond
the scope of this PR. This can be tracked as a follow-up improvement.
--
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]
