You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/09/27 06:59:01 UTC
[GitHub] [iceberg] rizaon opened a new pull request, #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
rizaon opened a new pull request, #5862:
URL: https://github.com/apache/iceberg/pull/5862
This attempt to deflake testWeakFileIOReferenceCleanUp by removing the failed assertion and changing the cache executor from `ForkJoinPool#commonPool` to `Runnable#run`.
Close #5861
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rizaon commented on pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
rizaon commented on PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#issuecomment-1260144441
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r981241109
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -250,6 +236,18 @@ private Table createTable(Map<String, String> properties) throws Exception {
.create();
}
+ protected HadoopCatalog hadoopCatalog(Map<String, String> catalogProperties) throws IOException {
Review Comment:
can probably be made private
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,22 +181,15 @@ public void testWeakFileIOReferenceCleanUp() {
}
}
- System.gc();
+ GcFinalization.awaitFullGc();
Review Comment:
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
##########
versions.props:
##########
@@ -10,6 +10,7 @@ org.apache.pig:pig = 0.14.0
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
Review Comment:
nit: you should be able to replace the previous line with `com.google.guava:* = 31.1-jre`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rizaon commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
rizaon commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r981169482
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,27 +182,27 @@ public void testWeakFileIOReferenceCleanUp() {
System.gc();
manifestCache.cleanUp();
- awaitQuiescence();
- Assert.assertEquals(maxIO, manifestCache.estimatedSize());
Assert.assertEquals(maxIO, manifestCache.stats().loadCount());
- Assert.assertEquals(
- "No entries should be evicted before IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT exceeded.",
- 0,
- manifestCache.stats().evictionCount());
// Insert one more FileIO to trigger cache eviction.
FileIO lastIO = cacheEnabledHadoopFileIO();
ContentCache lastCache = contentCache(manifestCache, lastIO);
- System.gc();
- manifestCache.cleanUp();
- awaitQuiescence();
+
+ int numGC = 0;
+ int maxGC = 100;
+ while (manifestCache.estimatedSize() > 2 && numGC < maxGC) {
+ System.gc();
Review Comment:
Replaced it with `GcFinalization.awaitFullGc` in rebased commit, [a0204a3](https://github.com/apache/iceberg/pull/5862/commits/a0204a38955188c70e2dcc7f9b3ff2a7e74fc949).
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r980941101
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,27 +182,27 @@ public void testWeakFileIOReferenceCleanUp() {
System.gc();
manifestCache.cleanUp();
- awaitQuiescence();
- Assert.assertEquals(maxIO, manifestCache.estimatedSize());
Assert.assertEquals(maxIO, manifestCache.stats().loadCount());
- Assert.assertEquals(
- "No entries should be evicted before IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT exceeded.",
- 0,
- manifestCache.stats().evictionCount());
// Insert one more FileIO to trigger cache eviction.
FileIO lastIO = cacheEnabledHadoopFileIO();
ContentCache lastCache = contentCache(manifestCache, lastIO);
- System.gc();
- manifestCache.cleanUp();
- awaitQuiescence();
+
+ int numGC = 0;
+ int maxGC = 100;
+ while (manifestCache.estimatedSize() > 2 && numGC < maxGC) {
+ System.gc();
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rizaon commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
rizaon commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r981397881
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,22 +181,15 @@ public void testWeakFileIOReferenceCleanUp() {
}
}
- System.gc();
+ GcFinalization.awaitFullGc();
Review Comment:
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](https://github.com/apache/iceberg/pull/5862/commits/572f0ca0322f0974ad0b31ac4c184bce8b91ef84), I simplified testWeakFileIOReferenceCleanUp by not exceeding IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rizaon commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
rizaon commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r989725507
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,31 +181,20 @@ public void testWeakFileIOReferenceCleanUp() {
}
}
- System.gc();
- manifestCache.cleanUp();
- awaitQuiescence();
- Assert.assertEquals(maxIO, manifestCache.estimatedSize());
- Assert.assertEquals(maxIO, manifestCache.stats().loadCount());
- Assert.assertEquals(
- "No entries should be evicted before IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT exceeded.",
- 0,
- manifestCache.stats().evictionCount());
-
- // Insert one more FileIO to trigger cache eviction.
+ // Insert the last FileIO and trigger GC + cleanup.
FileIO lastIO = cacheEnabledHadoopFileIO();
ContentCache lastCache = contentCache(manifestCache, lastIO);
- System.gc();
+ GcFinalization.awaitFullGc();
Review Comment:
Done in 713679b
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks merged pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
danielcweeks merged PR #5862:
URL: https://github.com/apache/iceberg/pull/5862
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r983142950
##########
bundled-guava/src/main/java/org/apache/iceberg/GuavaClasses.java:
##########
@@ -98,5 +99,6 @@ public class GuavaClasses {
CountingOutputStream.class.getName();
Suppliers.class.getName();
Stopwatch.class.getName();
+ GcFinalization.class.getName();
Review Comment:
@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?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rizaon commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
rizaon commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r981400664
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -250,6 +236,18 @@ private Table createTable(Map<String, String> properties) throws Exception {
.create();
}
+ protected HadoopCatalog hadoopCatalog(Map<String, String> catalogProperties) throws IOException {
Review Comment:
Done.
##########
versions.props:
##########
@@ -10,6 +10,7 @@ org.apache.pig:pig = 0.14.0
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
Review Comment:
Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r980941101
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,27 +182,27 @@ public void testWeakFileIOReferenceCleanUp() {
System.gc();
manifestCache.cleanUp();
- awaitQuiescence();
- Assert.assertEquals(maxIO, manifestCache.estimatedSize());
Assert.assertEquals(maxIO, manifestCache.stats().loadCount());
- Assert.assertEquals(
- "No entries should be evicted before IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT exceeded.",
- 0,
- manifestCache.stats().evictionCount());
// Insert one more FileIO to trigger cache eviction.
FileIO lastIO = cacheEnabledHadoopFileIO();
ContentCache lastCache = contentCache(manifestCache, lastIO);
- System.gc();
- manifestCache.cleanUp();
- awaitQuiescence();
+
+ int numGC = 0;
+ int maxGC = 100;
+ while (manifestCache.estimatedSize() > 2 && numGC < maxGC) {
+ System.gc();
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #5862: Core: Deflake TestManifestCaching.testWeakFileIOReferenceCleanUp
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #5862:
URL: https://github.com/apache/iceberg/pull/5862#discussion_r989671005
##########
core/src/test/java/org/apache/iceberg/TestManifestCaching.java:
##########
@@ -178,31 +181,20 @@ public void testWeakFileIOReferenceCleanUp() {
}
}
- System.gc();
- manifestCache.cleanUp();
- awaitQuiescence();
- Assert.assertEquals(maxIO, manifestCache.estimatedSize());
- Assert.assertEquals(maxIO, manifestCache.stats().loadCount());
- Assert.assertEquals(
- "No entries should be evicted before IO_MANIFEST_CACHE_MAX_FILEIO_DEFAULT exceeded.",
- 0,
- manifestCache.stats().evictionCount());
-
- // Insert one more FileIO to trigger cache eviction.
+ // Insert the last FileIO and trigger GC + cleanup.
FileIO lastIO = cacheEnabledHadoopFileIO();
ContentCache lastCache = contentCache(manifestCache, lastIO);
- System.gc();
+ GcFinalization.awaitFullGc();
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org