wenjin272 commented on code in PR #944:
URL: https://github.com/apache/flink-agents/pull/944#discussion_r3740950034
##########
python/flink_agents/runtime/flink_runner_context.py:
##########
@@ -752,14 +752,16 @@ def config(self) -> ReadableConfiguration:
@override
def close(self) -> None:
- if self.long_term_memory is not None:
- self.long_term_memory.close()
-
- if self.__resource_cache is not None:
- try:
- self.__resource_cache.close()
- finally:
- self.__resource_cache = None
+ ltm = self.__ltm
+ self.__ltm = None
+ try:
+ if ltm is not None:
+ ltm.close()
+ finally:
+ resource_cache = self.__resource_cache
+ self.__resource_cache = None
+ if resource_cache is not None:
+ resource_cache.close()
Review Comment:
If both `ltm.close()` and `resource_cache.close()` fail, Python's `finally`
semantics make the resource-cache exception primary and leave the earlier LTM
failure only in `__context__`. This contradicts the PR's stated behavior of
preserving the first exception and suppressing later ones, and it differs from
the Java `IOUtils.closeAll` paths. Could we explicitly aggregate these failures
and add a test where both closes throw?
--
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]
