You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "liferoad (via GitHub)" <gi...@apache.org> on 2024/02/11 01:45:48 UTC

[PR] fixed the display issue for impersonateServiceAccount with Java [beam]

liferoad opened a new pull request, #30283:
URL: https://github.com/apache/beam/pull/30283

   **Please** add a meaningful description for your change here
   
   ------------------------
   
   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)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30283:
URL: https://github.com/apache/beam/pull/30283#issuecomment-1937840288

   Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`


-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #30283:
URL: https://github.com/apache/beam/pull/30283


-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1487987185


##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java:
##########
@@ -488,6 +488,7 @@ public static void main(String[] args) throws Exception {
         "%s cannot be main() class with beam_fn_api enabled",
         StreamingDataflowWorker.class.getSimpleName());
 
+    LOG.info("Creating StreamingDataflowWorker from options: {}", options);

Review Comment:
   Changed this to debug. It will be useful when debugging the options used directly to create the streaming worker.



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1488215353


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -144,6 +171,8 @@ public static void main(Function<String, String> environmentVarGetter) throws Ex
     }
 
     System.out.format("Pipeline options %s%n", pipelineOptionsJson);
+    pipelineOptionsJson = removeNestedKey(pipelineOptionsJson, "impersonateServiceAccount");

Review Comment:
   agree with you. I just opened #30301 for some future improvements.



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1488065606


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -144,6 +171,8 @@ public static void main(Function<String, String> environmentVarGetter) throws Ex
     }
 
     System.out.format("Pipeline options %s%n", pipelineOptionsJson);
+    pipelineOptionsJson = removeNestedKey(pipelineOptionsJson, "impersonateServiceAccount");

Review Comment:
   OK I now realize why this one is doing a weird thing to the string of the JSON, which is not a good pattern.
   
   You _could_ have just set the pipeline option to `null` after parsing it. But of course then you would have to take a dependency on GCP pipeline options when the SDK harness should not have a dependency on GCP.
   
   So it is a hack. What should really happen is the Dataflow service strips this option before sending the options to the worker. Maybe what you actually want is for the Dataflow pipeline to have two sets of options in the job submission: (1) the options used to submit the job (2) the options used to run the job.
   
   Anyhow, please keep a bug open for this and make an extensive comment here. Because even though you didn't add a dependency on the GCP library, you did actually add a dependency on a GCP concept, plus a hack that someone who comes later will wonder "what is going on here?"



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1485637874


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -107,6 +111,29 @@ private static Endpoints.ApiServiceDescriptor getApiServiceDescriptor(String des
     return apiServiceDescriptorBuilder.build();
   }
 
+  public static String removeNestedKey(String jsonString, String keyToRemove) throws Exception {

Review Comment:
   Not sure when we can put this json util function. Any suggestion is welcome.



##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -107,6 +111,29 @@ private static Endpoints.ApiServiceDescriptor getApiServiceDescriptor(String des
     return apiServiceDescriptorBuilder.build();
   }
 
+  public static String removeNestedKey(String jsonString, String keyToRemove) throws Exception {

Review Comment:
   Not sure where we can put this json util function. Any suggestion is welcome.



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1486475358


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -107,6 +111,29 @@ private static Endpoints.ApiServiceDescriptor getApiServiceDescriptor(String des
     return apiServiceDescriptorBuilder.build();
   }
 
+  public static String removeNestedKey(String jsonString, String keyToRemove) throws Exception {

Review Comment:
   or leave it here until you need to move it, better



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "liferoad (via GitHub)" <gi...@apache.org>.
liferoad commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1487987758


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -107,6 +111,29 @@ private static Endpoints.ApiServiceDescriptor getApiServiceDescriptor(String des
     return apiServiceDescriptorBuilder.build();
   }
 
+  public static String removeNestedKey(String jsonString, String keyToRemove) throws Exception {

Review Comment:
   Sounds good. Thanks. Leave it here for now.



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1486474737


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -107,6 +111,29 @@ private static Endpoints.ApiServiceDescriptor getApiServiceDescriptor(String des
     return apiServiceDescriptorBuilder.build();
   }
 
+  public static String removeNestedKey(String jsonString, String keyToRemove) throws Exception {

Review Comment:
   You could just put it in `org.apache.beam.sdk.util` package somewhere. We have a ton of random junk there.



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1486826728


##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingDataflowWorker.java:
##########
@@ -488,6 +488,7 @@ public static void main(String[] args) throws Exception {
         "%s cannot be main() class with beam_fn_api enabled",
         StreamingDataflowWorker.class.getSimpleName());
 
+    LOG.info("Creating StreamingDataflowWorker from options: {}", options);

Review Comment:
   Is this a debug leftover or consider change to LOG.debug? I see sometimes pipeline options contain sensitive info (server ip, credential file name, etc, if user defined their own pipeline options) so we may refrain from log full pipeline option.



##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -107,6 +111,29 @@ private static Endpoints.ApiServiceDescriptor getApiServiceDescriptor(String des
     return apiServiceDescriptorBuilder.build();
   }
 
+  public static String removeNestedKey(String jsonString, String keyToRemove) throws Exception {

Review Comment:
   Yeah, leave it is fine. Seemingly it introduced a new dependency jackson_databind, but since this is also a dependency of java_core, it does not matter



-- 
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] fixed the display issue for impersonateServiceAccount with Java [beam]

Posted by "kennknowles (via GitHub)" <gi...@apache.org>.
kennknowles commented on code in PR #30283:
URL: https://github.com/apache/beam/pull/30283#discussion_r1488067223


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -144,6 +171,8 @@ public static void main(Function<String, String> environmentVarGetter) throws Ex
     }
 
     System.out.format("Pipeline options %s%n", pipelineOptionsJson);
+    pipelineOptionsJson = removeNestedKey(pipelineOptionsJson, "impersonateServiceAccount");

Review Comment:
   My comment sounds too negative - I mean to say that I am OK with this but we need to track the follow-up, make a clear comment, and handle it in Dataflow better.



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