mxm commented on a change in pull request #11084: [BEAM-9474] Improve
robustness of BundleFactory and ProcessEnvironment
URL: https://github.com/apache/beam/pull/11084#discussion_r390003729
##########
File path:
runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java
##########
@@ -166,11 +168,20 @@ public static DefaultJobBundleFactory create(
CacheBuilder.newBuilder()
.removalListener(
(RemovalNotification<Environment, WrappedSdkHarnessClient>
notification) -> {
- int refCount = notification.getValue().unref();
- LOG.debug(
- "Removed environment {} with {} remaining bundle
references.",
- notification.getKey(),
- refCount);
+ WrappedSdkHarnessClient client = notification.getValue();
Review comment:
It doesn't work though if we do not ensure dereferencing under all
circumstances. We need a safeguard here, also considering other runners may not
dereference correctly. Generally, it is hard to guarantee dereferencing due to
the nesting of DoFnRunners which may not even allow closing the bundle in error
cases. I considered not doing this but I think it is the safer route.
If you take a step back, when would the reference counting really be useful?
Every restarted job will anyways run in a new classloader, so the environment
will never be recycled. When we call close we should tear down everything.
Taking back another step, the reference counting should really be removed
entirely. It was error prone from the beginning leading to subtle problems with
dereferencing. If you don't mind, I'd remove it. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
