Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin merged PR #23917: URL: https://github.com/apache/flink/pull/23917 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on PR #23917: URL: https://github.com/apache/flink/pull/23917#issuecomment-1858772184 thanks for taking a look -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428571302
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
state match {
Review Comment:
suddenly.. :see_no_evil: so far I thought it was
`baseCheckpointPath.toFile.deleteOnExit();`, thanks and sorry about that!
now changed it
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428571302
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
state match {
Review Comment:
suddenly.. :see_no_evil: so far I thought it was
`baseCheckpointPath.toFile.deleteOnExit();`, thanks !
now changed it
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
Jiabao-Sun commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428219337
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
state match {
Review Comment:
Sorry, if I was wrong, please correct me.
I still don't quite understand why we need to delete the folder we just
created immediately.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
Jiabao-Sun commented on code in PR #23917: URL: https://github.com/apache/flink/pull/23917#discussion_r1428209269 ## flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala: ## @@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode) extends StreamingTestB super.before() // set state backend -// subfolder are managed here because the tests could fail during cleanup when concurrently executed (see FLINK-33820) -baseCheckpointPath = TempDirUtils.newFolder(tempFolder) +val baseCheckpointPath = Files.createTempDirectory(getClass.getCanonicalName) +Files.deleteIfExists(baseCheckpointPath); Review Comment: It's ok to use `Files.createTempDirectory(getClass.getCanonicalName)`. `TempDirUtils.newFolder(tempFolder)` may still cause problems, it's my fault of PR 33641. -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428204897
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
state match {
Review Comment:
Since `baseCheckpointPath` is used only in this method I do not see any
reason to have it as a field, so I just moved it completely to the method
since there `deleteIfExists` i think there is no need for
`ileUtils.deleteDirectory(baseCheckpointPath) m` so it could be removed
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428204897
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
state match {
Review Comment:
Since `baseCheckpointPath` is used only in this method I do not see any
reason to have it as a field, so I just moved it completely to the method
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428199378
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
Review Comment:
>baseCheckpointPath = Files.createTempDirectory("junit")
I didn't get why can't we use `baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)` ?
Havig classname in prefix could simplify finding root cause in case of
issues IMHO
The idea is to use `Files.createTempDirectory` and `deleteIfExists` to be
sure that delete is happening at the end and it should not intersect with
checkpoint removal
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
Jiabao-Sun commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1428188735
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
Review Comment:
```suggestion
baseCheckpointPath = Files.createTempDirectory("junit")
```
I noticed that previous failed stack trace:
```
Suppressed: java.nio.file.NoSuchFileException:
/tmp/junit8233404746490819295/junit2880192188533757139/71dc52714210ccdbd137bbcffa7955b6/chk-3
at
sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
at
sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
at
sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
at
sun.nio.fs.UnixFileAttributeViews$Basic.readAttributes(UnixFileAttributeViews.java:55)
at
sun.nio.fs.UnixFileSystemProvider.readAttributes(UnixFileSystemProvider.java:144)
at
sun.nio.fs.LinuxFileSystemProvider.readAttributes(LinuxFileSystemProvider.java:99)
at java.nio.file.Files.readAttributes(Files.java:1737)
at
java.nio.file.FileTreeWalker.getAttributes(FileTreeWalker.java:219)
at java.nio.file.FileTreeWalker.visit(FileTreeWalker.java:276)
at java.nio.file.FileTreeWalker.next(FileTreeWalker.java:372)
at java.nio.file.Files.walkFileTree(Files.java:2706)
at java.nio.file.Files.walkFileTree(Files.java:2742)
at
org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.deleteAllFilesAndDirectories(TempDirectory.java:329)
at
org.junit.jupiter.engine.extension.TempDirectory$CloseablePath.close(TempDirectory.java:310)
... 96 more
Suppressed: java.nio.file.NoSuchFileException:
/tmp/junit8233404746490819295/junit2880192188533757139/71dc52714210ccdbd137bbcffa7955b6/chk-3
at
sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
at
sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
at
sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
at
sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:244)
at
sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103)
at java.nio.file.Files.delete(Files.java:1126)
at
org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.resetPermissionsAndTryToDeleteAgain(TempDirectory.java:382)
at
org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.visitFileFailed(TempDirectory.java:342)
at
org.junit.jupiter.engine.extension.TempDirectory$CloseablePath$1.visitFileFailed(TempDirectory.java:329)
at java.nio.file.Files.walkFileTree(Files.java:2672)
... 99 more
```
The problem seems to have some clues. The failure to junit clean up
`TempDir` may be caused by the attempt to delete it concurrently with the
`CheckpointCoordinator` cleaning up completed Checkpoints.
https://github.com/apache/flink/blob/45f966e8c3c5e903b3843391874f7d2478122d8c/flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java#L1387-L1390
We can use `baseCheckpointPath = Files.createTempDirectory("junit")` to
avoid using junit to clean up the `TempForlder`. This will be safe for
upgrading junit to version 5.10.1.
By the way, the root cause of the previous deletion failed is likely to be
caused by the automatic cleaning complete checkpoint by
`Checkpointcoordinator`. I believe we can solve it soon. 樂️
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
Jiabao-Sun commented on code in PR #23917:
URL: https://github.com/apache/flink/pull/23917#discussion_r1427932231
##
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/runtime/utils/StreamingWithStateTestBase.scala:
##
@@ -68,8 +69,8 @@ class StreamingWithStateTestBase(state: StateBackendMode)
extends StreamingTestB
super.before()
// set state backend
-// subfolder are managed here because the tests could fail during cleanup
when concurrently executed (see FLINK-33820)
-baseCheckpointPath = TempDirUtils.newFolder(tempFolder)
+val baseCheckpointPath =
Files.createTempDirectory(getClass.getCanonicalName)
+Files.deleteIfExists(baseCheckpointPath);
state match {
Review Comment:
Thanks @snuyanzin for this hard work.
But I am a little confused, why is `baseCheckpointPath` defined as a stack
variable, so does the object variable `baseCheckpointPath: File = _` have no
effect anymore?
I suspect that there are still some problems about reuse the `tempFolder` by
`TempDirUtils.newFolder(tempFolder)` instead of directly using
`Files.createTempDirectory("junit")`.
I think the cleanup of `tempFolder` may still fail when
`FileUtils.deleteDirectory(baseCheckpointPath)` met
`DirectoryNotEmptyException` because it is possible that there are some threads
still writing to the folder.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on PR #23917: URL: https://github.com/apache/flink/pull/23917#issuecomment-1857613116 @Jiabao-Sun can you please have a look one more time PS. I started to use ```scala val baseCheckpointPath = Files.createTempDirectory(getClass.getCanonicalName) Files.deleteIfExists(baseCheckpointPath); ``` since for junit 5.10.1 it keeps failing as https://dev.azure.com/apache-flink/apache-flink/_build/results?buildId=55518=logs=0c940707-2659-5648-cbe6-a1ad63045f0a=075c2716-8010-5565-fe08-3c4bb45824a4=12635 -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on PR #23917: URL: https://github.com/apache/flink/pull/23917#issuecomment-1854237160 interesting, retry of successful build does not work... will sumbit same commit with different comment -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin commented on PR #23917: URL: https://github.com/apache/flink/pull/23917#issuecomment-1854127737 @flinkbot run azure -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
TanYuxin-tyx commented on PR #23917: URL: https://github.com/apache/flink/pull/23917#issuecomment-1853158768 LGTM -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
flinkbot commented on PR #23917: URL: https://github.com/apache/flink/pull/23917#issuecomment-1853042178 ## CI report: * 3247f8b2702cf14cde943067c8eaab53bd5dd2b1 UNKNOWN Bot commands The @flinkbot bot supports the following commands: - `@flinkbot run azure` re-run the last Azure build -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] [FLINK-33807] Update JUnit to 5.10.1 [flink]
snuyanzin opened a new pull request, #23917: URL: https://github.com/apache/flink/pull/23917 ## What is the purpose of the change Bump junit to 5.10.1 This could improve logging for https://github.com/apache/flink/pull/23914#pullrequestreview-1777970575 since it was improved on JUnit level at https://github.com/junit-team/junit5/pull/3249/files#diff-722173375ef777095d4d3ba36f4e5063f25c21f23701ad4cbd470018c2888fa8 https://github.com/junit-team/junit5/issues/3236 ## Verifying this change ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (yes ) - 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, Kubernetes/Yarn, 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) -- 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-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
