-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[Playground] Resolve issue with SCIO examples failing on start due to a timeout #24946
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Run RAT PreCommit |
33cd2fa to
2150f54
Compare
|
Assigning reviewers. If you would like to opt out of this review, comment R: @Abacn for label build. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
3ad333b to
2150f54
Compare
Run sbt tool during container build to let it download Scala dependencies form Maven during build time instead of having to wait for downloading all dependencies during first run of examples in container
… better imitate real deployments
2150f54 to
eecba19
Compare
| # Enable mitmproxy | ||
| ENV HTTP_PROXY="http://127.0.0.1:8081" | ||
| ENV HTTPS_PROXY="http://127.0.0.1:8081" | ||
| ENV SBT_OPTS="-Xmx512M -XX:+UseG1GC -XX:+UseStringDeduplication" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain again why we need mitmproxy? I read the website briefly and it looks like a networking tool. However, aren't we achieving our networking needs using kubernetes Service and other related kinds? Can our images and the Dockerfiles that describe them serve just to ship the binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's being used to filter out which Google Cloud Storage buckets are accessible by the examples.
However, after investigating a little bit it seems that this is not the best place to have it. We discussed this with @MakarkinSAkvelon and think that the best solution would be to have a single proxy node which would act as a reverse proxy for letting external connections to the runners and also as a single point though which runners would be able to only access the allowlisted hosts.
This approach with a single proxy node will simplify origin policy for frontend and we will be able to get rid of the need to generate config.g.dart completely.
This issue, however, I think, is beyond the scope of this PR and we can address it separately.
884c6fb to
d645408
Compare
damondouglas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR at commit d645408223 has the following test failures:
- go test beam.apache.org/playground/backend/internal/code_processing
=== RUN Test_getRunOrTestCmd/Get_run_cmd
code_processing_test.go:632: getExecuteCmd() = runCommand arg1, want runCommand arg1
--- FAIL: Test_getRunOrTestCmd/Get_run_cmd (0.00s)
=== RUN Test_getRunOrTestCmd/Get_test_cmd
code_processing_test.go:632: getExecuteCmd() = testCommand arg1 , want testCommand arg1
--- FAIL: Test_getRunOrTestCmd/Get_test_cmd (0.00s)
- go test beam.apache.org/playground/backend/internal/setup_tools/life_cycle
=== RUN TestSetup/Successfully_setup_life_cycle_with_Scio_sdk
life_cycle_setuper_test.go:342: Setup() error = error during create necessary files for the Scio sdk: exit status 1, wantErr false
--- FAIL: TestSetup/Successfully_setup_life_cycle_with_Scio_sdk (0.01s)
FYI this was a compute engine instance but nonetheless a clean environment.
Instead of the emulator, have you tried to:
If you still want to use the emulator, I recommend the tests that need it to actually activate and shut down the emulator but personally when I test Google api dependent code in the context of Go, I implement the grpc server. This doesn't replace integration tests but serves as a quicker way to detect errors in code. |
|
@damondouglas |
|
Reminder, please take a look at this pr: @Abacn |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
3ff0e7a to
6d85ebd
Compare
|
Mostly LGTM other than a few small comments |
Co-authored-by: Danny McCormick <dannymccormick@google.com>
Co-authored-by: Danny McCormick <dannymccormick@google.com>
|
I'm waiting on CI checks to pass before merging, probably will check back tomorrow |
SCIO examples timeout when it's a first run in the container. The reason is that
sbttool downloads Scala dependencies upon the first run, which happens synchronously and takes a significant amount of time.Downloading the Scala dependencies during the container build time mitigates this problem as
sbtwill use already downloaded files instead of fetching them from a remote host.Fixes issue with excessive memory usage, examples now can be run under 1GB of memory usage.
Also this PR resolves issue with execution environment not being properly removed after the example execution.
resolves #24943
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.