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/02/22 15:02:24 UTC
[GitHub] [pulsar] lhotari opened a new pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
lhotari opened a new pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673
### Motivation
`org.apache.pulsar.functions.LocalRunner` currently has several issues:
* cleanup and shutdown isn't handled in tests
* it doesn't support .nar files that aren't on the classpath
* adding support for .nar files that aren't on the classpath will help in the PIP-62 migration so that an integration test can run tests with a .nar file that has been built in some other build (for example)
### Modifications
- Make LocalRunner work properly with .nar files that aren't on the classpath
- also support the previous choice of providing the implementation classes
on the Thread context classloader classpath
- Pass nar file locations as system properties to tests:
- Pass pulsar-io cassandra and twitter nar file locations as system properties
to tests
- Pass pulsar-io-data-generator.nar and pulsar-functions-api-examples.jar
file locations as system properties to tests
- Improve LocalRunner cleanup/shutdown which wasn't handled before.
- Fix invalid test in PackagesApiTest
- Make PulsarFunction tests and Sink/Source tests work with .nar files which
aren't on the classpath.
- Extract FileServer in tests to simplify the test code
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791516890
/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] rdhabalia commented on pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
rdhabalia commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791102504
/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] merlimat merged pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-786112810
/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 a change in pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#discussion_r582909144
##########
File path: pulsar-broker/pom.xml
##########
@@ -308,14 +308,14 @@
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-functions-api-examples</artifactId>
<version>${project.version}</version>
- <scope>test</scope>
+ <scope>provided</scope>
Review comment:
actually when thinking more about it, changing from `test` to `provided` won't take it off the classpath as I have intended. I guess I should also change the type to `pom` so that there's a dependency to the project (so that `-am` works), but nothing gets added to the classpath. I'll revisit this.
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791700443
/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] codelipenghui commented on pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791225940
@nlu90 @freeznet could you please help also review this PR?
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-783709746
/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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-789953405
/pulsar-bot 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] sijie commented on a change in pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#discussion_r581246699
##########
File path: pulsar-broker/pom.xml
##########
@@ -308,14 +308,14 @@
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-functions-api-examples</artifactId>
<version>${project.version}</version>
- <scope>test</scope>
+ <scope>provided</scope>
Review comment:
Can you explain why did you change it from `test` scope to `provided` scope?
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-785967434
@jerrypeng would you mind reviewing this PR? thank you
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-789869578
/pulsarbot run-failure-check
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-784914269
@jerrypeng Would you mind reviewing this PR?
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791674896
/pulsarbot run-failure-check
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791618141
@merlimat This PR has been approved by @rdhabalia . Would you also mind checking this?
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#discussion_r581907537
##########
File path: pulsar-broker/pom.xml
##########
@@ -308,14 +308,14 @@
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-functions-api-examples</artifactId>
<version>${project.version}</version>
- <scope>test</scope>
+ <scope>provided</scope>
Review comment:
the reason for this is to get it off the classpath. There shouldn't be a need to have it on the classpath.
the reason to have the `provided` scope is to add a project dependency between this module and pulsar-functions-api-examples. `maven-dependency-plugin` is used to copy the `pulsar-functions-api-examples` jar file to the build 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] lhotari commented on pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791724676
/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 removed a comment on pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari removed a comment on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791674896
/pulsarbot run-failure-check
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-789715972
/pulsarbot run-failure-check
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791161503
/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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791388485
/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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-785038317
> Can you handle "cleanup and shutdown isn't handled in tests" first?
I can do this, however I think there's a justification to put multiple related changes together because the Pulsar CI is in such bad shape. It's pretty painful to get even a single PR to pass all the flaky tests.
> I am not sure how the refactor will handle PIP-62 yet. Can we defer that until it is actually happening? Let's avoid over-engineering now.
There's no need to couple these changes with PIP-62. This PR fixes clear problems in LocalRunner. What makes you think that this is over-engineering?
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791685815
/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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-788828706
@sijie @merlimat @david-streamlio @jerrypeng @jiazhai @codelipenghui
Please review this change and provide feedback.
This change is important for improving the build and Pulsar GitHub Actions based CI.
One benefit is that it simplifies the building of Pulsar since it reduces the dependencies of pulsar-broker module since it gets rid of the usage of maven-antrun-plugin plugin for copying the .nar files and pulsar-functions-api-examples.jar file to src/test/resources. This is needed so that it's possible to reuse binary artifacts in the Pulsar CI pipeline. For example, in the unit test jobs, it should be possible to reuse the binary artifacts that are built in a previous step instead of building everything with "mvn clean install" in each step. When it's possible to reuse binary artifacts, we can save about 15 minutes for each unit test and integration test build job in the GitHub Action based CI pipeline.
In addition to these benefits, this PR fixes real bugs and problems in LocalRunner. Those are described in the description of this PR.
Since the improvements of Pulsar CI are blocked by this PR, I'm asking for more reviews and addressable feedback. Thank you
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#discussion_r582947261
##########
File path: pulsar-broker/pom.xml
##########
@@ -308,14 +308,14 @@
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-functions-api-examples</artifactId>
<version>${project.version}</version>
- <scope>test</scope>
+ <scope>provided</scope>
Review comment:
I now pushed a fix which reverts the change from test to provided, but instead changes the type to `pom` so that the classes aren't added to the classpath at all.
If the classes are on the classpath, it wouldn't be possible to test loading the functions from a file or URI since the classes would always get loaded from the parent classloader which would contain the classes. That is one of the rationals to make the changes in this PR.
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791717942
/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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari edited a comment on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-785038317
> Can you handle "cleanup and shutdown isn't handled in tests" first?
Hi @sijie . Thanks for the review.
I can do this, however I think there's a justification to put multiple related changes together because the Pulsar CI is in such bad shape. It's pretty painful to get even a single PR to pass all the flaky tests.
> I am not sure how the refactor will handle PIP-62 yet. Can we defer that until it is actually happening? Let's avoid over-engineering now.
There's no need to couple these changes with PIP-62. This PR fixes clear problems in LocalRunner. What makes you think that this is over-engineering?
----------------------------------------------------------------
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 #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-791669825
/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 removed a comment on pull request #9673: [Testing] Improve LocalRunner to support .nar files that aren't on the classpath
Posted by GitBox <gi...@apache.org>.
lhotari removed a comment on pull request #9673:
URL: https://github.com/apache/pulsar/pull/9673#issuecomment-784914269
@jerrypeng Would you mind reviewing this PR?
----------------------------------------------------------------
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