[FLINK-14522] Introduce JavaGcCleanerWrapper to find Java GC Cleaner depending on JVM version #9997

azagrebin · 2019-10-25T14:07:16Z

What is the purpose of the change

sun.misc.Cleaner is not available since Java 9.

It was moved to jdk.internal.ref.Cleaner of java.base module (Open JDK-8148117), but another new public API was introduced to achieve the same behaviour: java.lang.ref.Cleaner;

A popular solution is use reflection to look up for the location of the Cleaner class depending on running JVM version.

Brief change log

Introduce JavaGcCleanerWrapper which uses reflection to look up and provide proper Cleaner implementation depending on JVM version on runtime
Introduce JavaGcCleanerWrapperTest to test that the clean method is called

Verifying this change

Run JavaGcCleanerWrapperTest

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

flinkbot · 2019-10-25T14:10:47Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 765f629 (Wed Dec 04 14:53:45 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2019-10-25T14:37:39Z

CI report:

fef7b76 : SUCCESS Build
060ca00 : SUCCESS Build
765f629 : SUCCESS Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

GJL · 2019-10-25T14:48:28Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+
+		if (foundCleanerClass == null) {
+			throw new FlinkRuntimeException(
+				String.format("A proper Java Cleaner class is not found among %s", CLEANER_PROVIDERS),


public static void main(String[] args) { System.out.println(Arrays.asList(Foo.INSTANCE)); } private enum Foo { INSTANCE }

output:

[INSTANCE]

Do you think CLEANER_PROVIDERS should be an enum?

Error will be [...] among [INSTANCE, INSTANCE]

True, I will convert it to the list of CleanerProvider::getCleanerClassName

GJL · 2019-10-25T15:17:50Z

flink-core/src/test/java/org/apache/flink/util/JavaGcCleanerWrapperTest.java

+		AtomicInteger callCounter = new AtomicInteger();
+		Runnable cleaner = JavaGcCleanerWrapper.create(new Object(), callCounter::incrementAndGet);
+		System.gc();
+		Thread.sleep(100); // more chance for GC to run


Can you explain this test? It would also pass without System.gc().

I will rename the test: testCleanOperationRunsOnlyOnceEitherOnGcOrExplicitly.
If GC does not run immediately (it should run 99%) then explicit cleaner running will do the clean operation. Also the cleaner lambda should not capture the owner object in the error log message, I will fix it.

The pause is too long imo.

tillrohrmann

Thanks a lot for this PR @azagrebin. The changes look good to me. I had some minor comments which you could address while merging the PR. +1 for merging if you've tested this code also with Java 11.

tillrohrmann · 2019-10-29T16:57:27Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+ *
+ * <p>The wrapper looks up the underlying Java GC Cleaner class in different packages
+ */
+public class JavaGcCleanerWrapper {


Can be enum or needs a private constructor as an utility class.

tillrohrmann · 2019-10-29T16:58:08Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+	private static final Logger LOG = LoggerFactory.getLogger(JavaGcCleanerWrapper.class);
+
+	private static final String LEGACY_CLEANER_CLASS_NAME = "sun.misc.Cleaner"; // before Java 9
+	private static final String JAVA9_CLEANER_CLASS_NAME = "java.lang.ref.Cleaner"; // since Java 9+


Shouldn't these fields be part of the implementation of the different CleanerProviders?

tillrohrmann · 2019-10-29T16:59:51Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+	private interface CleanerProvider {
+		String getCleanerClassName();
+
+		CleanerFactory createCleanerFactory(Class<?> cleanerClass);


The interface is a bit strange that the provider knows about the class name, but needs to get the Class<?> passed into the createCleanerFactory. Couldn't the createCleanerFactory method do the class lookup?

tillrohrmann · 2019-10-29T17:00:40Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+		}
+	}
+
+	private static class LegacyCleanerFactory implements CleanerFactory {


final missing.

tillrohrmann · 2019-10-29T17:00:46Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+		}
+	}
+
+	private static class Java9CleanerFactory implements CleanerFactory {


final missing.

tillrohrmann · 2019-10-30T10:12:46Z

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

+				throw new FlinkRuntimeException("Failed to find Java 9 Cleaner$Cleanable#clean method", e);
+			}
+			return new Java9CleanerFactory(cleaner, cleanerRegisterMethod, cleanMethod);
+		}


This method could be a bit cleaner if we factor out the individual steps into methods which are responsible for the error handling.

flink-core/src/main/java/org/apache/flink/util/JavaGcCleanerWrapper.java

tillrohrmann · 2019-10-30T10:20:58Z

flink-core/src/test/java/org/apache/flink/util/JavaGcCleanerWrapperTest.java

+		AtomicInteger callCounter = new AtomicInteger();
+		Runnable cleaner = JavaGcCleanerWrapper.create(new Object(), callCounter::incrementAndGet);
+		System.gc();
+		Thread.sleep(100); // more chance for GC to run


The pause is too long imo.

…depending on JVM version

azagrebin · 2019-10-30T14:03:51Z

Thanks for the reviews @tillrohrmann @GJL
I addressed comments, will merge when the CI is green

GJL · 2019-10-30T14:52:34Z

flink-core/src/test/java/org/apache/flink/util/JavaGcCleanerWrapperTest.java

+		AtomicInteger callCounter = new AtomicInteger();
+		Runnable cleaner = JavaGcCleanerWrapper.create(new Object(), callCounter::incrementAndGet);
+		System.gc(); // not guaranteed to be run always but should in practice
+		Thread.sleep(10); // more chance for GC to run


Just an idea: CountDownLatch has a method boolean await(long timeout, TimeUnit unit)

True, I just wanted to avoid test instabilities if GC is not called for some reason at once.

rmetzger added the review=description? label Oct 25, 2019

GJL reviewed Oct 25, 2019

View reviewed changes

rmetzger added the component=Runtime/Task label Oct 25, 2019

GJL reviewed Oct 25, 2019

View reviewed changes

azagrebin force-pushed the FLINK-14522 branch from fef7b76 to 060ca00 Compare October 29, 2019 14:15

tillrohrmann self-assigned this Oct 29, 2019

tillrohrmann approved these changes Oct 30, 2019

View reviewed changes

[FLINK-14522] Introduce JavaGcCleanerWrapper to find Java GC Cleaner …

765f629

…depending on JVM version

azagrebin force-pushed the FLINK-14522 branch from 375bb34 to 765f629 Compare October 30, 2019 14:02

GJL reviewed Oct 30, 2019

View reviewed changes

azagrebin merged commit e41c0fe into apache:master Oct 31, 2019

azagrebin mentioned this pull request Nov 21, 2019

[FLINK-14901] Throw Error in MemoryUtils if there is problem with using system classes over reflection #10282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-14522] Introduce JavaGcCleanerWrapper to find Java GC Cleaner depending on JVM version #9997

[FLINK-14522] Introduce JavaGcCleanerWrapper to find Java GC Cleaner depending on JVM version #9997

azagrebin commented Oct 25, 2019 •

edited

flinkbot commented Oct 25, 2019 •

edited

flinkbot commented Oct 25, 2019 •

edited

GJL Oct 25, 2019 •

edited

azagrebin Oct 29, 2019

GJL Oct 29, 2019

azagrebin Oct 29, 2019

GJL Oct 25, 2019

azagrebin Oct 29, 2019

tillrohrmann Oct 30, 2019

tillrohrmann left a comment •

edited

tillrohrmann Oct 29, 2019

tillrohrmann Oct 29, 2019

tillrohrmann Oct 29, 2019

tillrohrmann Oct 29, 2019

tillrohrmann Oct 29, 2019

tillrohrmann Oct 30, 2019

tillrohrmann Oct 30, 2019

azagrebin commented Oct 30, 2019

GJL Oct 30, 2019

azagrebin Oct 30, 2019

[FLINK-14522] Introduce JavaGcCleanerWrapper to find Java GC Cleaner depending on JVM version #9997

[FLINK-14522] Introduce JavaGcCleanerWrapper to find Java GC Cleaner depending on JVM version #9997

Conversation

azagrebin commented Oct 25, 2019 • edited

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Oct 25, 2019 • edited

Automated Checks

Review Progress

flinkbot commented Oct 25, 2019 • edited

CI report:

GJL Oct 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azagrebin commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azagrebin commented Oct 25, 2019 •

edited

flinkbot commented Oct 25, 2019 •

edited

flinkbot commented Oct 25, 2019 •

edited

GJL Oct 25, 2019 •

edited

tillrohrmann left a comment •

edited