You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/03/24 11:57:37 UTC

[GitHub] [pulsar] lhotari opened a new pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

lhotari opened a new pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032


   ### Motivation
   
   - Existing solution used in Pulsar Functions tests doesn't support testForkCount > 1 since there is code like this in many tests:
   ```
           // delete all function temp files
           File dir = new File(System.getProperty("java.io.tmpdir"));
           File[] foundFiles = dir.listFiles((ignoredDir, name) -> name.startsWith("function"));
   
           for (File file : foundFiles) {
               file.delete();
           }
   ```
   
   - After running Pulsar tests, there are extracted nar directories left in /tmp directory. This is a sign that proper cleanup isn't happening.
   
   
   ### Modifications
   
   - Use unique temporary nar extraction directory for each test
     - Delete directory consistently in AfterMethod
   
   - Add feature LocalRunner to create a unique temp directory for extracting nar files. Delete directory on close
   
   - Fix some ClassLoader resource cleanup issues that came up while working on the changes


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-808324796


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-807977062


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-808313068


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805762897


   @jerrypeng @david-streamlio please review this PR. It's a change in Pulsar Functions 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-808038064


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] aahmed-se commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
aahmed-se commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805767217


   We should be cleaning the env when the test starts not when it ends, there is no value in cleanup then.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806935644


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806516434


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806379970


   > Generally looks good. Added some come comments. Thank you for working on this.
   
   Thanks for the review @jerrypeng . I'll soon push changes to address your review comments about the deleted files check (I'll bring that back).


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805782751


   > I noticed that there's a gap in this PR. There also needs to be handling for these temp files:
   > 
   > https://github.com/apache/pulsar/blob/602137fa1b87bf3fdb7a99f7fc096d77c0fe40f4/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java#L766
   > 
   > https://github.com/apache/pulsar/blob/c27f7a340a3c78a3a5004d08b8ac05ed99672ee3/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SourcesImpl.java#L744
   > 
   > https://github.com/apache/pulsar/blob/c27f7a340a3c78a3a5004d08b8ac05ed99672ee3/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SinksImpl.java#L748
   
   I addressed the cleanup of these files by using the nar extraction directory as the temporary directory for these files. I think that it should be ok for that purpose. Adding yet another configuration parameter for the downloaded files would be adding too much overhead.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on a change in pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#discussion_r601093223



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/io/PulsarFunctionE2ETest.java
##########
@@ -457,12 +458,6 @@ private void testE2EPulsarFunction(String jarFilePathUrl) throws Exception {
             // make sure subscriptions are cleanup
             assertEquals(admin.topics().getStats(sourceTopic).subscriptions.size(), 0);
         });
-
-        // make sure all temp files are deleted
-        File dir = new File(System.getProperty("java.io.tmpdir"));
-        File[] foundFiles = dir.listFiles((dir1, name) -> name.startsWith("function"));
-
-        Assert.assertEquals(foundFiles.length, 0, "Temporary files left over: " + Arrays.asList(foundFiles));

Review comment:
       The check is back.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on a change in pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#discussion_r601063158



##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -196,18 +200,37 @@ public LocalRunner(FunctionConfig functionConfig, SourceConfig sourceConfig, Sin
         this.runtimeEnv = runtimeEnv;
         this.secretsProviderClassName = secretsProviderClassName;
         this.secretsProviderConfig = secretsProviderConfig;
-        this.narExtractionDirectory = narExtractionDirectory != null ? narExtractionDirectory
-                : NarClassLoader.DEFAULT_NAR_EXTRACTION_DIR;
+        if (narExtractionDirectory != null) {

Review comment:
       The change in behavior is intentional for LocalRunner. The intent of the changes are that unless narExtractionDirectory is specified, a temporary directory will be created which is deleted when `LocalRunner.close()` is called. Do you think that this type of behavior is unacceptable? I hope it would be ok to do this change since it reduces the chance that temporary files get left in the main system temp directory.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] jerrypeng commented on a change in pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#discussion_r600932380



##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -196,18 +200,37 @@ public LocalRunner(FunctionConfig functionConfig, SourceConfig sourceConfig, Sin
         this.runtimeEnv = runtimeEnv;
         this.secretsProviderClassName = secretsProviderClassName;
         this.secretsProviderConfig = secretsProviderConfig;
-        this.narExtractionDirectory = narExtractionDirectory != null ? narExtractionDirectory
-                : NarClassLoader.DEFAULT_NAR_EXTRACTION_DIR;
+        if (narExtractionDirectory != null) {

Review comment:
       We are changing the existing behavior here. If the narExtractionDirectory is not specified we should default to NarClassLoader.DEFAULT_NAR_EXTRACTION_DIR




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806422415


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806793636


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805935569


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-808296050


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari edited a comment on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805781265


   > We should be cleaning the env when the test starts not when it ends, there is no value in cleanup then.
   
   Usually resources should be cleaned up and not left around. Isn't this the case?
   @aahmed-se  Please review the changes in this PR so that there's more context.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-808231258


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-807241774


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] eolivelli commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805940589


   @aahmed-se 
   
   We cannot use the same directory and clean it up, otherwise we are not able to support concurrent tests.
   Now we are using a fresh new directory, so there is no need to clean up.
   It is good that we clean up in the end of the test in order to save resources (CI/localenv disk space).
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-807930106


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] jerrypeng commented on a change in pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#discussion_r601876264



##########
File path: pulsar-functions/localrun/src/main/java/org/apache/pulsar/functions/LocalRunner.java
##########
@@ -196,18 +200,37 @@ public LocalRunner(FunctionConfig functionConfig, SourceConfig sourceConfig, Sin
         this.runtimeEnv = runtimeEnv;
         this.secretsProviderClassName = secretsProviderClassName;
         this.secretsProviderConfig = secretsProviderConfig;
-        this.narExtractionDirectory = narExtractionDirectory != null ? narExtractionDirectory
-                : NarClassLoader.DEFAULT_NAR_EXTRACTION_DIR;
+        if (narExtractionDirectory != null) {

Review comment:
       I think it is fine




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805766916


   I noticed that there's a gap in this PR. There also needs to be handling for these temp files:
   
   https://github.com/apache/pulsar/blob/602137fa1b87bf3fdb7a99f7fc096d77c0fe40f4/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java#L766
   
   https://github.com/apache/pulsar/blob/c27f7a340a3c78a3a5004d08b8ac05ed99672ee3/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SourcesImpl.java#L744
   
   https://github.com/apache/pulsar/blob/c27f7a340a3c78a3a5004d08b8ac05ed99672ee3/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/SinksImpl.java#L748


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on a change in pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#discussion_r601061199



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/io/PulsarFunctionE2ETest.java
##########
@@ -457,12 +458,6 @@ private void testE2EPulsarFunction(String jarFilePathUrl) throws Exception {
             // make sure subscriptions are cleanup
             assertEquals(admin.topics().getStats(sourceTopic).subscriptions.size(), 0);
         });
-
-        // make sure all temp files are deleted
-        File dir = new File(System.getProperty("java.io.tmpdir"));
-        File[] foundFiles = dir.listFiles((dir1, name) -> name.startsWith("function"));
-
-        Assert.assertEquals(foundFiles.length, 0, "Temporary files left over: " + Arrays.asList(foundFiles));

Review comment:
       Thanks for the advice @jerrypeng . I'll revisit the changes and return the checks.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-808146059


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-807995143


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806406613


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806436290


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-806403475


   @jerrypeng I have addressed the review comments. PTAL


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-807479822


   /pulsarbot run-failure-checks


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] eolivelli merged pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032


   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] lhotari commented on pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#issuecomment-805781265


   > We should be cleaning the env when the test starts not when it ends, there is no value in cleanup then.
   
   Usually resources should be cleaned up and not left around. Isn't this the case?
   Please review the changes in this PR so that there's more context.


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [pulsar] jerrypeng commented on a change in pull request #10032: [Testing] Improve Functions unit tests by using unique nar extraction directory

Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #10032:
URL: https://github.com/apache/pulsar/pull/10032#discussion_r600781946



##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/io/PulsarFunctionE2ETest.java
##########
@@ -457,12 +458,6 @@ private void testE2EPulsarFunction(String jarFilePathUrl) throws Exception {
             // make sure subscriptions are cleanup
             assertEquals(admin.topics().getStats(sourceTopic).subscriptions.size(), 0);
         });
-
-        // make sure all temp files are deleted
-        File dir = new File(System.getProperty("java.io.tmpdir"));
-        File[] foundFiles = dir.listFiles((dir1, name) -> name.startsWith("function"));
-
-        Assert.assertEquals(foundFiles.length, 0, "Temporary files left over: " + Arrays.asList(foundFiles));

Review comment:
       We should not delete this check because it verifies the temporary function files will get clean up.

##########
File path: pulsar-broker/src/test/java/org/apache/pulsar/io/PulsarFunctionE2ETest.java
##########
@@ -457,12 +458,6 @@ private void testE2EPulsarFunction(String jarFilePathUrl) throws Exception {
             // make sure subscriptions are cleanup
             assertEquals(admin.topics().getStats(sourceTopic).subscriptions.size(), 0);
         });
-
-        // make sure all temp files are deleted
-        File dir = new File(System.getProperty("java.io.tmpdir"));
-        File[] foundFiles = dir.listFiles((dir1, name) -> name.startsWith("function"));
-
-        Assert.assertEquals(foundFiles.length, 0, "Temporary files left over: " + Arrays.asList(foundFiles));

Review comment:
       Based on your PR, you should at least check the download directory to make sure there are not files in 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org