You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "chamikaramj (via GitHub)" <gi...@apache.org> on 2023/10/03 00:38:09 UTC
[PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
chamikaramj opened a new pull request, #28783:
URL: https://github.com/apache/beam/pull/28783
Also performs following fixes.
* Transform service controller waits for expansion services to start before performing expansion or discovery operations.
* When expansion requests fail, transform service controller sends back a detailed error with errors from all expansion services instead of just sending the last failure.
Issue is https://github.com/apache/beam/issues/26833.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1753451507
Run Java_GCP_IO_Direct PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on code in PR #28783:
URL: https://github.com/apache/beam/pull/28783#discussion_r1349814718
##########
sdks/java/transform-service/src/main/java/org/apache/beam/sdk/transformservice/ExpansionService.java:
##########
@@ -40,6 +47,12 @@ public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplB
final List<Endpoints.ApiServiceDescriptor> endpoints;
+ private boolean checkedAllServices = false;
+
+ private static final long SERVICE_CHECK_TIMEOUT = 60000;
Review Comment:
Done.
##########
sdks/java/transform-service/launcher/src/main/java/org/apache/beam/sdk/transformservice/launcher/TransformServiceLauncher.java:
##########
@@ -124,10 +128,84 @@ private TransformServiceLauncher(@Nullable String projectName, int port) throws
}
}
+ // Setting up the dependencies directory.
+ File dependenciesDir = Paths.get(tmpDir, "dependencies_dir").toFile();
+ Path updatedRequirementsFilePath = Paths.get(dependenciesDir.toString(), "requirements.txt");
+ LOG.info(
+ "Creating a temporary directory for storing dependencies: "
Review Comment:
Done.
##########
sdks/java/transform-service/launcher/src/test/java/org/apache/beam/sdk/transformservice/launcher/TransformServiceLauncherTest.java:
##########
@@ -0,0 +1,185 @@
+/*
Review Comment:
Thanks. Added the projects to the PreCommit task.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1756410675
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1756413837
Run TransformService_Direct PostCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on code in PR #28783:
URL: https://github.com/apache/beam/pull/28783#discussion_r1353225798
##########
sdks/java/transform-service/src/main/java/org/apache/beam/sdk/transformservice/ExpansionService.java:
##########
@@ -48,10 +61,61 @@ public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplB
clientFactory != null ? clientFactory : DEFAULT_EXPANSION_SERVICE_CLIENT_FACTORY;
}
+ // Waits till all expansion services are ready.
+ private void waitForAllServicesToBeReady() throws TimeoutException {
+ if (disableServiceCheck) {
+ // Service check disabled. Just returning.
+ return;
+ }
+
+ outer:
+ for (Endpoints.ApiServiceDescriptor endpoint : endpoints) {
+ long start = System.currentTimeMillis();
+ long duration = 10;
+ while (System.currentTimeMillis() - start < SERVICE_CHECK_TIMEOUT) {
+ try {
+ String url = endpoint.getUrl();
+ int portIndex = url.lastIndexOf(":");
+ int port = portIndex > 0 ? Integer.parseInt(url.substring(portIndex + 1)) : 80;
Review Comment:
Thanks. Updated.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1756635996
Run Python_PVR_Flink PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1753451621
Run Python_Transforms PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28783:
URL: https://github.com/apache/beam/pull/28783#discussion_r1353181001
##########
sdks/java/transform-service/src/main/java/org/apache/beam/sdk/transformservice/ExpansionService.java:
##########
@@ -48,10 +61,61 @@ public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplB
clientFactory != null ? clientFactory : DEFAULT_EXPANSION_SERVICE_CLIENT_FACTORY;
}
+ // Waits till all expansion services are ready.
+ private void waitForAllServicesToBeReady() throws TimeoutException {
+ if (disableServiceCheck) {
+ // Service check disabled. Just returning.
+ return;
+ }
+
+ outer:
+ for (Endpoints.ApiServiceDescriptor endpoint : endpoints) {
+ long start = System.currentTimeMillis();
+ long duration = 10;
+ while (System.currentTimeMillis() - start < SERVICE_CHECK_TIMEOUT) {
+ try {
+ String url = endpoint.getUrl();
+ int portIndex = url.lastIndexOf(":");
+ int port = portIndex > 0 ? Integer.parseInt(url.substring(portIndex + 1)) : 80;
Review Comment:
Changes LGTM. Have you been able to check this 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1756635679
Run Portable_Python PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1756735731
Thanks. All tests are passing.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1743983445
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1753323829
Run Java PreCommit
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1752274296
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/28783?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#28783](https://app.codecov.io/gh/apache/beam/pull/28783?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6e67b14) into [master](https://app.codecov.io/gh/apache/beam/commit/5e38decf9e723a385057131b01bbd33d8c60bda3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5e38dec) will **decrease** coverage by `0.02%`.
> Report is 83 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #28783 +/- ##
==========================================
- Coverage 72.21% 72.20% -0.02%
==========================================
Files 684 685 +1
Lines 101241 101527 +286
==========================================
+ Hits 73116 73312 +196
- Misses 26546 26636 +90
Partials 1579 1579
```
| [Flag](https://app.codecov.io/gh/apache/beam/pull/28783/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [python](https://app.codecov.io/gh/apache/beam/pull/28783/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.70% <0.00%> (-0.07%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/beam/pull/28783?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...on/apache\_beam/utils/transform\_service\_launcher.py](https://app.codecov.io/gh/apache/beam/pull/28783?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvdHJhbnNmb3JtX3NlcnZpY2VfbGF1bmNoZXIucHk=) | `23.07% <0.00%> (-1.37%)` | :arrow_down: |
... and [22 files with indirect coverage changes](https://app.codecov.io/gh/apache/beam/pull/28783/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj merged PR #28783:
URL: https://github.com/apache/beam/pull/28783
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #28783:
URL: https://github.com/apache/beam/pull/28783#issuecomment-1743979667
R: @Abacn
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates the Transform Service to accept Python extra packages through the Java API [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #28783:
URL: https://github.com/apache/beam/pull/28783#discussion_r1346118024
##########
sdks/java/transform-service/src/main/java/org/apache/beam/sdk/transformservice/ExpansionService.java:
##########
@@ -40,6 +47,12 @@ public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplB
final List<Endpoints.ApiServiceDescriptor> endpoints;
+ private boolean checkedAllServices = false;
+
+ private static final long SERVICE_CHECK_TIMEOUT = 60000;
Review Comment:
SERVICE_CHECK_TIMEOUT_MILLIS or add a comment about the unit?
##########
sdks/java/transform-service/launcher/src/main/java/org/apache/beam/sdk/transformservice/launcher/TransformServiceLauncher.java:
##########
@@ -124,10 +128,84 @@ private TransformServiceLauncher(@Nullable String projectName, int port) throws
}
}
+ // Setting up the dependencies directory.
+ File dependenciesDir = Paths.get(tmpDir, "dependencies_dir").toFile();
+ Path updatedRequirementsFilePath = Paths.get(dependenciesDir.toString(), "requirements.txt");
+ LOG.info(
+ "Creating a temporary directory for storing dependencies: "
Review Comment:
minor - this LOG may better fit under the else branch of `if (!dependenciesDir.mkdir())` below. When dependenciesDir.exists() its not actually creating but just reusing
##########
sdks/java/transform-service/launcher/src/test/java/org/apache/beam/sdk/transformservice/launcher/TransformServiceLauncherTest.java:
##########
@@ -0,0 +1,185 @@
+/*
Review Comment:
It appears this test (and unit test in sdks/java/transformservice project) is not executed in any precommit?
##########
sdks/java/transform-service/src/main/java/org/apache/beam/sdk/transformservice/ExpansionService.java:
##########
@@ -48,10 +61,61 @@ public class ExpansionService extends ExpansionServiceGrpc.ExpansionServiceImplB
clientFactory != null ? clientFactory : DEFAULT_EXPANSION_SERVICE_CLIENT_FACTORY;
}
+ // Waits till all expansion services are ready.
+ private void waitForAllServicesToBeReady() throws TimeoutException {
+ if (disableServiceCheck) {
+ // Service check disabled. Just returning.
+ return;
+ }
+
+ outer:
+ for (Endpoints.ApiServiceDescriptor endpoint : endpoints) {
+ long start = System.currentTimeMillis();
+ long duration = 10;
+ while (System.currentTimeMillis() - start < SERVICE_CHECK_TIMEOUT) {
+ try {
+ String url = endpoint.getUrl();
+ int portIndex = url.lastIndexOf(":");
+ int port = portIndex > 0 ? Integer.parseInt(url.substring(portIndex + 1)) : 80;
Review Comment:
Is default port 80 documented somewhere / rely on some other components ? If I remember correctly the port for expansion service is dynamically signed?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org