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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
