Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
chia7712 commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1518692934
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
+ */
+public static File tempRelativeDir(String root) {
+File rootFile = new File(root);
+boolean created = rootFile.mkdir();
+
+File result = tempDirectory(rootFile.toPath(), null);
+if (created) {
+rootFile.deleteOnExit();
Review Comment:
> Have you verified that the order of these independent removals is correct
though?
I had run the tests with this PR, and the folder created by this helper gets
removed as expected. see
https://github.com/apache/kafka/pull/15289#pullrequestreview-1923156750
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
ijuma commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1518680219
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
+ */
+public static File tempRelativeDir(String root) {
+File rootFile = new File(root);
+boolean created = rootFile.mkdir();
+
+File result = tempDirectory(rootFile.toPath(), null);
+if (created) {
+rootFile.deleteOnExit();
Review Comment:
Have you verified that the order of these independent removals is correct
though? I don't think there's any guarantee - that's my point.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
chia7712 commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1518650602
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
+ */
+public static File tempRelativeDir(String root) {
+File rootFile = new File(root);
+boolean created = rootFile.mkdir();
+
+File result = tempDirectory(rootFile.toPath(), null);
+if (created) {
+rootFile.deleteOnExit();
Review Comment:
> This is not enough to ensure the file is removed - it will only happen if
the directory is empty.
the sub-folder created by `tempDirectory(rootFile.toPath(), null)` will get
removed so the `rootFile` should be empty and it is removable when exit. If
callers want to create another file under passed `root` manually, they should
not use this helper.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
ijuma commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1518649065
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
+ */
+public static File tempRelativeDir(String root) {
+File rootFile = new File(root);
+boolean created = rootFile.mkdir();
+
+File result = tempDirectory(rootFile.toPath(), null);
+if (created) {
+rootFile.deleteOnExit();
Review Comment:
This is not enough to ensure the file is removed - it will only happen if
the directory is empty.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
chia7712 merged PR #15289: URL: https://github.com/apache/kafka/pull/15289 -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
gaurav-narula commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1516589314
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
+ */
+public static File tempRelativeDir(String root) {
+File rootFile = new File(root);
+boolean created = rootFile.mkdir();
+
+File result = tempDirectory(rootFile.toPath(), null);
+if (created) {
+rootFile.deleteOnExit();
Review Comment:
Force pushed the updates, PTAL again
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
chia7712 commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1516374859
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
Review Comment:
Could you please complete the docs?
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -195,6 +195,25 @@ public static File tempDirectory() {
return tempDirectory(null);
}
+/**
+ * Create a temporary directory under the given root directory.
+ * The root directory is removed on JVM exit if it doesn't already exist
+ * when this function is invoked.
+ *
+ * @param root path to create temporary directory under
+ * @return
+ */
+public static File tempRelativeDir(String root) {
+File rootFile = new File(root);
+boolean created = rootFile.mkdir();
+
+File result = tempDirectory(rootFile.toPath(), null);
+if (created) {
+rootFile.deleteOnExit();
Review Comment:
How about using shorter version:
```java
public static File tempRelativeDir(String root) {
File rootFile = new File(root);
if (rootFile.mkdir()) rootFile.deleteOnExit();
return tempDirectory(rootFile.toPath(), null);
}
```
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
gaurav-narula commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1516017140
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,9 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
Review Comment:
> if we decide to cleanup parent folder also, we should do same thing for
java code.
I've made the following changes in
[3af0426](https://github.com/apache/kafka/pull/15289/commits/3af0426481cedd79f6f827317c9f144ea68fb8cb):
* Scala TestUtils now delegates to the function in JTestUtils
* The function is modified such that we delete the `rootDir` on JVM exit if
it didn't exist prior to the function being invoked.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
chia7712 commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1511231627
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,9 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
Review Comment:
>I feel it's semantically better for the function that creates parentFile to
handle its deletion rather than delegating it to JTestUtils.tempDirectory since
it might not be very obvious to callers of JTestUtils.tempDirectory otherwise.
I agree that `JTestUtils.tempDirectory` is not obvious about deleting.
However, it seems to me that is the problem. Callers can't ensure the duty of
"cleanup", and so maybe the consistent behavior is more important here - if we
decide to cleanup parent folder also, we should do same thing for java code.
BTW, we can have a better naming if we assume `tempRelativeDir` needs to
delete parent 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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
gaurav-narula commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1511199142
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,9 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
Review Comment:
Note that `JTestUtils.tempDirectory` is overloaded with `parentFile`
possibly being null. In those cases, the cleanup works as intended.
This is however a special case where `parentFile != null`, because the test
create a logdir in a "relative location". `JTestUtils.tempDirectory` does
handle the removal of everything it creates under `parentFile` but it doesn't
remove the `parentFile` though.
I feel it's semantically better for the function that creates `parentFile`
to handle its deletion rather than delegating it to `JTestUtils.tempDirectory`
since it might not be very obvious to callers of `JTestUtils.tempDirectory`
otherwise.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
chia7712 commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1511059328
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,9 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
Review Comment:
How about improving the `JTestUtils.tempDirectory` directly? That all
java/scala code can get benefit:
1. they don't need to create/delete temp folder manually
2. All folders created by `JTestUtils.tempDirectory` can get deleted after
tests get completed.
3. eliminate potential flaky encountering failure due to data corruption
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
gaurav-narula commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1507510343
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,18 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
+
+parentFile.deleteOnExit()
+Exit.addShutdownHook("delete-temp-log-dir-on-exit", {
+ try {
+Utils.delete(parentFile)
Review Comment:
Good point - turns we can just use `parentFile.deleteOnExit()`. Updated in
[394f2e3](https://github.com/apache/kafka/pull/15289/commits/394f2e30e3ac2110921f8e1ea2978c405194a11b)
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,18 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
+
+parentFile.deleteOnExit()
+Exit.addShutdownHook("delete-temp-log-dir-on-exit", {
+ try {
+Utils.delete(parentFile)
Review Comment:
Good point - turns out we can just use `parentFile.deleteOnExit()`. Updated
in
[394f2e3](https://github.com/apache/kafka/pull/15289/commits/394f2e30e3ac2110921f8e1ea2978c405194a11b)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
soarez commented on code in PR #15289:
URL: https://github.com/apache/kafka/pull/15289#discussion_r1489605994
##
core/src/test/scala/unit/kafka/utils/TestUtils.scala:
##
@@ -137,7 +137,18 @@ object TestUtils extends Logging {
val parentFile = new File(parent)
parentFile.mkdirs()
-JTestUtils.tempDirectory(parentFile.toPath, null)
+val result = JTestUtils.tempDirectory(parentFile.toPath, null)
+
+parentFile.deleteOnExit()
+Exit.addShutdownHook("delete-temp-log-dir-on-exit", {
+ try {
+Utils.delete(parentFile)
Review Comment:
Do we need both `parentFile.deleteOnExit()` and `Utils.delete(parentFile)`
on the shutdown hook?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: Cleanup log.dirs in ReplicaManagerTest on JVM exit [kafka]
gaurav-narula commented on PR #15289: URL: https://github.com/apache/kafka/pull/15289#issuecomment-1917579474 Addressed in [4406670](https://github.com/apache/kafka/pull/15289/commits/4406670459210eb7fac009c836288955c7bcb23c) > Shouldn't we instead update the tests to use temp directories and cleanup any files they create? I think the tests deliberately use relative directories because of KAFKA-5759. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
