gemini-code-assist[bot] commented on code in PR #38611:
URL: https://github.com/apache/beam/pull/38611#discussion_r3293714691
##########
sdks/python/apache_beam/utils/subprocess_server.py:
##########
@@ -186,53 +192,45 @@ def __exit__(self, *unused_args):
self.stop()
def start(self):
Review Comment:
The retry logic (previously 3 attempts) has been removed from the `start`
method. While the PR addresses specific timeout issues by improving environment
staging, removing retries makes the `SubprocessServer` less resilient to other
transient startup failures, such as temporary port unavailability or system
load spikes. It is recommended to keep the retry loop while incorporating the
new `stop_force()` cleanup logic on each failure to maintain robustness.
##########
sdks/python/apache_beam/runners/portability/expansion_service_main.py:
##########
@@ -73,15 +71,9 @@ def main(argv):
artifact_service.ArtifactRetrievalService(
artifact_service.BeamFilesystemHandler(None).file_reader),
server)
- # Ensure gRPC server successfully binds. If this fails (e.g., due to port
collision),
- # add_insecure_port returns 0. We raise an error to crash the subprocess
immediately,
- # allowing the parent process to detect it and fail fast rather than
hanging.
- bound_port = server.add_insecure_port(address)
- if not bound_port:
- raise RuntimeError(
- "Failed to bind expansion service to {}".format(address))
+ server.add_insecure_port(address)
server.start()
- _LOGGER.info('Listening for expansion requests at %d', bound_port)
+ _LOGGER.info('Listening for expansion requests at %d', known_args.port)
Review Comment:
The check for the return value of `add_insecure_port` has been removed. If
the server fails to bind to the requested port (e.g., due to a port collision),
`add_insecure_port` returns `0`. Without this check, the server will proceed to
start but won't be listening on the expected port, leading to silent failures.
Additionally, logging `known_args.port` can be misleading if an ephemeral port
(0) was requested or if binding failed. Using the returned `bound_port` ensures
the log reflects the actual state.
```python
bound_port = server.add_insecure_port(address)
if not bound_port:
raise RuntimeError(
"Failed to bind expansion service to {}".format(address))
server.start()
_LOGGER.info('Listening for expansion requests at %d', bound_port)
```
--
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]
