[
https://issues.apache.org/jira/browse/BEAM-3327?focusedWorklogId=93384&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-93384
]
ASF GitHub Bot logged work on BEAM-3327:
----------------------------------------
Author: ASF GitHub Bot
Created on: 20/Apr/18 19:10
Start Date: 20/Apr/18 19:10
Worklog Time Spent: 10m
Work Description: bsidhom commented on a change in pull request #5189:
[BEAM-3327] Basic Docker environment factory
URL: https://github.com/apache/beam/pull/5189#discussion_r183144364
##########
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/FnApiControlClientPoolService.java
##########
@@ -79,7 +76,7 @@ public static FnApiControlClientPoolService
offeringClientsToPool(
// discarded, which should be performed by a call to #shutdownNow. The
remote caller must be
// able to handle an unexpectedly terminated connection.
vendedClients.add(newClient);
Review comment:
<!--thread_id:cc_182896326_t;
commit:1fd121da1417624b3b84f0300648251da64b9cb5; resolved:0-->
<!--section:context-quote-->
> **bsidhom** wrote:
> Ack. I've added synchronization around `vendedClients` itself. It may be
worth synchronizing the entire close method now that we're synchronizing over
the bulk of its work. @tgroh What do you think?
<!--section:body-->
Actually, I just realized that the list is already backed by a
CopyOnWriteArrayList and we're using the atomic boolean in close in
conjunction. However, if we want to protect against incoming clients getting
leaked after a call to close, we should do away with the atomic/COW and just
use synchronization where necessary.
Another note about leak prevention: While using the vended client list
prevents leakages of unclosed but invalid InstructionRequestHandlers, we're
also telling ControlClientPool consumers that they're responsible for cleanup.
So a consumer may terminate a control client and remove all references to it,
expecting it to be GC'd. However, clients will never be garbage collected until
the control server is shut down.
It seems plausible (depending on the runner) to have a long-lived control
server that is shared between jobs and that controls many different
environments during its lifespan. If we want to deal with that case, we may
need to hold only `WeakReference`s to clients. @tgroh Do we care about this
scenario? Are control servers even allowed to be reused between jobs?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
Issue Time Tracking
-------------------
Worklog Id: (was: 93384)
Time Spent: 13h 40m (was: 13.5h)
> Add abstractions to manage Environment Instance lifecycles.
> -----------------------------------------------------------
>
> Key: BEAM-3327
> URL: https://issues.apache.org/jira/browse/BEAM-3327
> Project: Beam
> Issue Type: New Feature
> Components: runner-core
> Reporter: Thomas Groh
> Assignee: Ben Sidhom
> Priority: Major
> Labels: portability
> Time Spent: 13h 40m
> Remaining Estimate: 0h
>
> This permits remote stage execution for arbitrary environments
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
