Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp by rizaon · Pull Request #5862 · apache/iceberg

rizaon · 2022-09-27T06:58:59Z

This attempt to deflake testWeakFileIOReferenceCleanUp by removing the failed assertion and changing the cache executor from ForkJoinPool#commonPool to Runnable#run.

Close #5861

nastra · 2022-09-27T08:39:14Z

core/src/test/java/org/apache/iceberg/TestManifestCaching.java

+    int numGC = 0;
+    int maxGC = 100;
+    while (manifestCache.estimatedSize() > 2 && numGC < maxGC) {
+      System.gc();


in general it's not a good practice to depend on repeated (or even single) System.gc() calls to make something work.
I would probably suggest to use https://github.com/google/guava/blob/master/guava-testlib/src/com/google/common/testing/GcFinalization.java#L297 once here instead of plain System.gc() calls in a loop. That's also what the Caffeine lib is using for their testing

Replaced it with GcFinalization.awaitFullGc in rebased commit, a0204a3.

I understand iceberg-bundled-guava relocate guava namespace from com.google.common to org.apache.iceberg.relocated.com.google.common. I don't know if and how to treat guava-testlib the same way.

nastra · 2022-09-27T13:24:24Z

core/src/test/java/org/apache/iceberg/TestManifestCaching.java

        .create();
  }

+  protected HadoopCatalog hadoopCatalog(Map<String, String> catalogProperties) throws IOException {


can probably be made private

nastra · 2022-09-27T13:25:42Z

versions.props

 com.fasterxml.jackson.*:* = 2.11.4
 com.google.errorprone:error_prone_annotations = 2.3.3
 com.google.guava:guava = 31.1-jre
+com.google.guava:guava-testlib = 31.1-jre


nit: you should be able to replace the previous line with com.google.guava:* = 31.1-jre

nastra · 2022-09-27T13:41:57Z

core/src/test/java/org/apache/iceberg/TestManifestCaching.java

    }

-    System.gc();
+    GcFinalization.awaitFullGc();


the comment in L188 says // Insert one more FileIO to trigger cache eviction. but aren't we triggering cache eviction already here?
It seems the original version of the test was expecting things to be evicted only when IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT is reached/exceeded

I think that comment I made is inherently flawed since I use System.gc().

The main purpose of the test is to verify that cache entry with garbage collected keys will be removed from the cache, and GcFinalization.awaitFullGc() + manifestCache.cleanUp() does evict those entries. In 572f0ca, I simplified testWeakFileIOReferenceCleanUp by not exceeding IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT.

…CACHE_MAX_FILEIO_DEFAULT

rizaon · 2022-09-27T22:35:50Z

Got the following error:

Error: eckstyle] [ERROR] /home/runner/work/iceberg/iceberg/core/src/test/java/org/apache/iceberg/TestManifestCaching.java:24:1: Use org.apache.iceberg.relocated.* classes from bundled-guava module instead. [BanUnrelocatedGuavaClasses]

I'm not sure how to include guava-testlib into iceberg-bundled-guava.

…tlib

nastra · 2022-09-29T07:07:00Z

bundled-guava/src/main/java/org/apache/iceberg/GuavaClasses.java

    CountingOutputStream.class.getName();
    Suppliers.class.getName();
    Stopwatch.class.getName();
+    GcFinalization.class.getName();


@rdblue any thoughts on adding a test class to GuavaClasses vs relaxing checkstyle so that classes from com.google.common.testing are allowed in tests?

I'd probably allow the direct classes in tests.

@rizaon for this you'd want to add something to our checktyle rules so that classes from com.google.common.testing are allowed to be used in tests directly rather than having to bundle them via GuavaClasses

Done in 3063b1a

Using the suppression like this would allow people to e.g. use com.google.common.base.Preconditions directly in tests right? We still want to make sure that classes from com.google.common are used via org.apache.iceberg.relocated.* in tests as well.
You might need to play a bit with the correct checkstyle configuration so that

all java code can't import com.google.common directly

test java code can import com.google.common.testing only

Please check if the rule breakdown in 3e06421 is OK.

rdblue · 2022-10-04T17:46:48Z

@danielcweeks, can you help review this as well since you reviewed the original PR?

nastra

this LGTM now, thanks for fixing @rizaon

danielcweeks · 2022-10-07T04:28:37Z

core/src/test/java/org/apache/iceberg/TestManifestCaching.java

    FileIO lastIO = cacheEnabledHadoopFileIO();
    ContentCache lastCache = contentCache(manifestCache, lastIO);
-    System.gc();
+    GcFinalization.awaitFullGc();


So this seems like a better approach, but it looks like the recommendation is to use awaitClear/awaitDone with some condition and timeout if possible. awaitFullGc appears to only wait for a single reference (probably something they construct as part of the test). Seems likely that we could still end up in a flaky test condition.

Done in 713679b

github-actions bot added the core label Sep 27, 2022

nastra reviewed Sep 27, 2022

View reviewed changes

Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp

a0204a3

rizaon force-pushed the master-Deflake_testWeakFileIOReferenceCleanUp branch from ca75b5a to a0204a3 Compare September 27, 2022 12:18

nastra reviewed Sep 27, 2022

View reviewed changes

github-actions bot added the build label Sep 27, 2022

Simplify testWeakFileIOReferenceCleanUp by not exceeding IO_MANIFEST_…

572f0ca

…CACHE_MAX_FILEIO_DEFAULT

rizaon added 3 commits September 28, 2022 06:40

Include GcFinalization into iceberg-bundled-guava

4062f96

Exclude com.google.errorprone from guava-testlib

7eed924

Exclude com.google.code.findbugs and com.google.j2objc from guava-tes…

ed5986c

…tlib

nastra reviewed Sep 29, 2022

View reviewed changes

rdblue requested a review from danielcweeks October 4, 2022 17:46

Supress BanUnrelocatedGuavaClasses for test files

3063b1a

github-actions bot added the INFRA label Oct 5, 2022

Add rule BanGuavaTesting

3e06421

nastra approved these changes Oct 6, 2022

View reviewed changes

danielcweeks reviewed Oct 7, 2022

View reviewed changes

Replace awaitFullGc with awaitDone

713679b

danielcweeks approved these changes Oct 7, 2022

View reviewed changes

danielcweeks merged commit 5eb78a4 into apache:master Oct 10, 2022

Conversation

rizaon commented Sep 27, 2022

Uh oh!

nastra Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rizaon commented Sep 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblue commented Oct 4, 2022

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nastra Sep 27, 2022 •

edited

Loading