You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "SabaSathya (via GitHub)" <gi...@apache.org> on 2023/06/27 00:47:19 UTC

[GitHub] [beam] SabaSathya opened a new pull request, #27256: Added FIRESTORE_HOST env variable to allow endpoint (i.e. host + port…

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

   …) other than default endpoint for testing purposes. If FIRESTORE_HOST env variable is set, it will be used as Firestore endpoint. If FIRESTORE_HOST env variable is not set, the default endpoint will be used.
   
   **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://beam.apache.org/contribute/get-started-contributing/#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.
   


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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/build.gradle:
##########
@@ -186,11 +186,13 @@ task integrationTest(type: Test, dependsOn: processTestResources) {
   def gcpProject = project.findProperty('gcpProject') ?: 'apache-beam-testing'
   def gcpTempRoot = project.findProperty('gcpTempRoot') ?: 'gs://temp-storage-for-end-to-end-tests'
   def firestoreDb = project.findProperty('firestoreDb') ?: 'firestoredb'
+  def host = project.findProperty('host') ?: 'batch-firestore.googleapis.com:443'

Review Comment:
   Keeping default.  Otherwise, the default value is not provided.



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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreOptions.java:
##########
@@ -57,4 +58,21 @@ public interface FirestoreOptions extends PipelineOptions {
 
   /** Set the Firestore database ID to connect to. */
   void setFirestoreDb(String firestoreDb);
+
+  /**
+   * A host port pair to allow connecting to a Cloud Firestore instead of the default live service.
+   *
+   * @return the string representation of a host and port pair to be used when constructing Cloud
+   *     Firestore clients.
+   */
+  @Nonnull

Review Comment:
   Set the default value for host.



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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -86,7 +90,11 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
 
       FirestoreOptions firestoreOptions = options.as(FirestoreOptions.class);
       String emulatorHostPort = firestoreOptions.getEmulatorHost();
+      if (emulatorHostPort == null) {
+        emulatorHostPort = System.getenv(FIRESTORE_EMULATOR_HOST_ENV_VARIABLE);
+      }
       if (emulatorHostPort != null) {
+        firestoreOptions.setEmulatorHost(emulatorHostPort);

Review Comment:
   Removed the update back in FirestoreOptions.



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

[GitHub] [beam] pcostell commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/it/BaseFirestoreIT.java:
##########
@@ -92,12 +92,12 @@ abstract class BaseFirestoreIT {
           .build();
 
   protected static String project;
-  protected GcpOptions options;
+  protected GcpOptions gcpOptions;

Review Comment:
   nit: Do we need this anymore? Or can we just extract the project below?



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -47,6 +47,10 @@
 @Immutable
 class FirestoreStatefulComponentFactory implements Serializable {
 
+  private static final String DEFAULT_FIRESTORE_HOST = "batch-firestore.googleapis.com:443";
+  private static final String FIRESTORE_HOST_ENV_VARIABLE = "FIRESTORE_HOST";

Review Comment:
   No longer used, right?



##########
sdks/java/io/google-cloud-platform/build.gradle:
##########
@@ -186,11 +186,13 @@ task integrationTest(type: Test, dependsOn: processTestResources) {
   def gcpProject = project.findProperty('gcpProject') ?: 'apache-beam-testing'
   def gcpTempRoot = project.findProperty('gcpTempRoot') ?: 'gs://temp-storage-for-end-to-end-tests'
   def firestoreDb = project.findProperty('firestoreDb') ?: 'firestoredb'
+  def host = project.findProperty('host') ?: 'batch-firestore.googleapis.com:443'

Review Comment:
   Consider leaving this unset by default (it would catch the bugs below where we assume it is set).



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -86,6 +90,9 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
 
       FirestoreOptions firestoreOptions = options.as(FirestoreOptions.class);
       String emulatorHostPort = firestoreOptions.getEmulatorHost();
+      if (emulatorHostPort == null) {

Review Comment:
   Can we drop this now too?



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -97,9 +104,13 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
                     .build());
       } else {
         GcpOptions gcpOptions = options.as(GcpOptions.class);
+        String host = firestoreOptions.getHost();
+        if (host == null) {
+          host = System.getenv().getOrDefault(FIRESTORE_HOST_ENV_VARIABLE, DEFAULT_FIRESTORE_HOST);

Review Comment:
   this should just be set to the default value now.



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreOptions.java:
##########
@@ -57,4 +58,21 @@ public interface FirestoreOptions extends PipelineOptions {
 
   /** Set the Firestore database ID to connect to. */
   void setFirestoreDb(String firestoreDb);
+
+  /**
+   * A host port pair to allow connecting to a Cloud Firestore instead of the default live service.
+   *
+   * @return the string representation of a host and port pair to be used when constructing Cloud
+   *     Firestore clients.
+   */
+  @Nonnull

Review Comment:
   In the stateful factory & test helper we interpret null as "use the default".
   
   Alternatively: we could set the default here in the options, so you can assume it is nonnull.



##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/it/FirestoreTestingHelper.java:
##########
@@ -125,17 +125,17 @@ enum DataLayout {
       "initialization.fields.uninitialized") // testClass and testName are managed via #apply
   public FirestoreTestingHelper(CleanupMode cleanupMode) {
     this.cleanupMode = cleanupMode;
-    options = TestPipeline.testingPipelineOptions().as(GcpOptions.class);
+    gcpOptions = TestPipeline.testingPipelineOptions().as(GcpOptions.class);
     firestoreBeamOptions =
         TestPipeline.testingPipelineOptions()
             .as(org.apache.beam.sdk.io.gcp.firestore.FirestoreOptions.class);
     firestoreOptions =
         FirestoreOptions.newBuilder()
-            .setCredentials(options.getGcpCredential())
-            .setProjectId(options.getProject())
+            .setCredentials(gcpOptions.getGcpCredential())
+            .setProjectId(gcpOptions.getProject())
             .setDatabaseId(firestoreBeamOptions.getFirestoreDb())
+            .setHost(firestoreBeamOptions.getHost())

Review Comment:
   I think we still need to handle unset here (I think this may only work because we are always setting it in the build file).



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

[GitHub] [beam] Abacn commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1610069831

   Hi, I disabled FirestoreV1IT again on master due to test failures: #27267
   
   I see the same test failure on https://ci-beam.apache.org/job/beam_PostCommit_Java_DataflowV1_PR/182/
   
   For testing this PR now it would need to remove the exclusion here:
   
   https://github.com/apache/beam/blob/664f355ee9c631afb4d80440e837e7b48028bbc8/runners/google-cloud-dataflow-java/build.gradle#L563
   
   https://github.com/apache/beam/blob/664f355ee9c631afb4d80440e837e7b48028bbc8/runners/google-cloud-dataflow-java/build.gradle#L612
   


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

[GitHub] [beam] github-actions[bot] commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

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

   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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/it/BaseFirestoreIT.java:
##########
@@ -92,12 +92,12 @@ abstract class BaseFirestoreIT {
           .build();
 
   protected static String project;
-  protected GcpOptions options;
+  protected GcpOptions gcpOptions;

Review Comment:
   Removed.



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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -97,9 +104,13 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
                     .build());
       } else {
         GcpOptions gcpOptions = options.as(GcpOptions.class);
+        String host = firestoreOptions.getHost();
+        if (host == null) {
+          host = System.getenv().getOrDefault(FIRESTORE_HOST_ENV_VARIABLE, DEFAULT_FIRESTORE_HOST);

Review Comment:
   Done.



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

[GitHub] [beam] SabaSathya commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "SabaSathya (via GitHub)" <gi...@apache.org>.
SabaSathya commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1608692438

   Run PostCommit_Java_Dataflow


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

[GitHub] [beam] bvolpato commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "bvolpato (via GitHub)" <gi...@apache.org>.
bvolpato commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1610717598

   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

[GitHub] [beam] SabaSathya commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "SabaSathya (via GitHub)" <gi...@apache.org>.
SabaSathya commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1611926341

   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

[GitHub] [beam] Abacn commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1631408000

   FirestoreV1IT integration tests need to be triggered by phrase `Run PostCommit_Java_Dataflow` The relevant tests is currently disabled: https://github.com/apache/beam/pull/27256#issuecomment-1610069831
   
   To confirm, is this PR still blocked by #25851 ?
   
   


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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -97,9 +100,11 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
                     .build());
       } else {
         GcpOptions gcpOptions = options.as(GcpOptions.class);
+        String host = System.getenv().getOrDefault(FIRESTORE_HOST_VARIABLE, DEFAULT_FIRESTORE_HOST);
+        firestoreOptions.setHost(host);

Review Comment:
   Revised to get env or default value only if there is no value in options.



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

[GitHub] [beam] pcostell commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -97,9 +100,11 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
                     .build());
       } else {
         GcpOptions gcpOptions = options.as(GcpOptions.class);
+        String host = System.getenv().getOrDefault(FIRESTORE_HOST_VARIABLE, DEFAULT_FIRESTORE_HOST);
+        firestoreOptions.setHost(host);

Review Comment:
   This doesn't seem right. We appear to always be ignoring any host set in the options.
   
   Shouldn't the logic mirror the emulatorHost logic? emulatorHost claims that it prefers 1) value in option then 2) value in env. I expect we should follow the same rules.



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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -47,6 +47,10 @@
 @Immutable
 class FirestoreStatefulComponentFactory implements Serializable {
 
+  private static final String DEFAULT_FIRESTORE_HOST = "batch-firestore.googleapis.com:443";
+  private static final String FIRESTORE_HOST_ENV_VARIABLE = "FIRESTORE_HOST";

Review Comment:
   Yes.  Removed.



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

[GitHub] [beam] github-actions[bot] commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

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

   Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
   
   R: @bvolpato for label java.
   R: @bvolpato for label io.
   
   Available commands:
   - `stop reviewer notifications` - opt out of the automated review tooling
   - `remind me after tests pass` - tag the comment author after tests pass
   - `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
   
   The PR bot will only process comments in the main thread (not review comments).


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

[GitHub] [beam] SabaSathya commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "SabaSathya (via GitHub)" <gi...@apache.org>.
SabaSathya commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1608690506

   Run PostCommit_Java_Dataflow 


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

[GitHub] [beam] SabaSathya commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "SabaSathya (via GitHub)" <gi...@apache.org>.
SabaSathya commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1608693466

   Run PostCommit_Java_DataflowV2


-- 
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] Enable FirestoreV1IT to run against a non default host [beam]

Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1745796802

   > This PR results in a command line arg of `--host`, which is a little generic and has the risk of conflicting with other `PipelineOptions` classes (not to mention non-PipelineOptions arguments). Can we revisit to rename to something more specific like `firestoreHost` (which corresponds with the already-existing `firestoreDb`)
   > 
   > 
   > 
   > cc @Abacn 
   
   Thanks, this sounds totally reasonable and I should have noticed this honestly. Would you mind drafting a PR for it. This flag currently is used for testing purpose so it should be 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.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

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

[GitHub] [beam] Abacn merged pull request #27256: Enable FirestoreV1IT to run against a non default host

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


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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -86,6 +90,9 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
 
       FirestoreOptions firestoreOptions = options.as(FirestoreOptions.class);
       String emulatorHostPort = firestoreOptions.getEmulatorHost();
+      if (emulatorHostPort == null) {

Review Comment:
   Done.



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

[GitHub] [beam] SabaSathya commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/it/FirestoreTestingHelper.java:
##########
@@ -125,17 +125,17 @@ enum DataLayout {
       "initialization.fields.uninitialized") // testClass and testName are managed via #apply
   public FirestoreTestingHelper(CleanupMode cleanupMode) {
     this.cleanupMode = cleanupMode;
-    options = TestPipeline.testingPipelineOptions().as(GcpOptions.class);
+    gcpOptions = TestPipeline.testingPipelineOptions().as(GcpOptions.class);
     firestoreBeamOptions =
         TestPipeline.testingPipelineOptions()
             .as(org.apache.beam.sdk.io.gcp.firestore.FirestoreOptions.class);
     firestoreOptions =
         FirestoreOptions.newBuilder()
-            .setCredentials(options.getGcpCredential())
-            .setProjectId(options.getProject())
+            .setCredentials(gcpOptions.getGcpCredential())
+            .setProjectId(gcpOptions.getProject())
             .setDatabaseId(firestoreBeamOptions.getFirestoreDb())
+            .setHost(firestoreBeamOptions.getHost())

Review Comment:
   Leaving it as it is 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

[GitHub] [beam] damccorm commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1631228165

   What are next steps on this PR? @SabaSathya @pcostell @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

[GitHub] [beam] pcostell commented on a diff in pull request #27256: Enable FirestoreV1IT to run against a non default host

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


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -86,7 +90,11 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
 
       FirestoreOptions firestoreOptions = options.as(FirestoreOptions.class);
       String emulatorHostPort = firestoreOptions.getEmulatorHost();
+      if (emulatorHostPort == null) {
+        emulatorHostPort = System.getenv(FIRESTORE_EMULATOR_HOST_ENV_VARIABLE);
+      }
       if (emulatorHostPort != null) {
+        firestoreOptions.setEmulatorHost(emulatorHostPort);

Review Comment:
   Injecting the value back into the value given to us does not make sense -- we should be treating the options argument as immutable.



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

[GitHub] [beam] SabaSathya commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "SabaSathya (via GitHub)" <gi...@apache.org>.
SabaSathya commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1608541375

   #27255 


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

[GitHub] [beam] SabaSathya commented on pull request #27256: Enable FirestoreV1IT to run against a non default host

Posted by "SabaSathya (via GitHub)" <gi...@apache.org>.
SabaSathya commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1612147207

   R: @pcostell for Firestore.
   


-- 
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] Enable FirestoreV1IT to run against a non default host [beam]

Posted by "kellen (via GitHub)" <gi...@apache.org>.
kellen commented on PR #27256:
URL: https://github.com/apache/beam/pull/27256#issuecomment-1745564522

   This PR results in a command line arg of `--host`, which is a little generic and has the risk of conflicting with other `PipelineOptions` classes (not to mention non-PipelineOptions arguments). Can we revisit to rename to something more specific like `firestoreHost` (which corresponds with the already-existing `firestoreDb`)
   
   cc @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