yixiaoshen commented on code in PR #36895:
URL: https://github.com/apache/beam/pull/36895#discussion_r2558116195
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreStatefulComponentFactory.java:
##########
@@ -94,12 +102,17 @@ FirestoreStub getFirestoreStub(PipelineOptions options) {
builder
.setCredentialsProvider(FixedCredentialsProvider.create(gcpOptions.getGcpCredential()))
.setEndpoint(firestoreOptions.getFirestoreHost());
+ String projectId =
+ configuredProjectId != null
+ ? configuredProjectId
+ : firestoreOptions.getFirestoreProject();
+ if (projectId == null) {
+ projectId = gcpOptions.getProject();
+ }
+ String databaseId =
+ configuredDatabaseId != null ? configuredDatabaseId :
firestoreOptions.getFirestoreDb();
Review Comment:
while you are at it, do you mind updating this comment:
https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/firestore/FirestoreOptions.java#L52
(remove 'Note: named database is currently an internal feature in Firestore.
Do not set this to anything other than "(default)".')
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/firestore/it/FirestoreTestingHelper.java:
##########
@@ -136,7 +136,7 @@ public FirestoreTestingHelper(CleanupMode cleanupMode) {
.setCredentials(gcpOptions.getGcpCredential())
.setProjectId(
firstNonNull(firestoreBeamOptions.getFirestoreProject(),
gcpOptions.getProject()))
- .setDatabaseId(firestoreBeamOptions.getFirestoreDb())
+ .setDatabaseId(firstNonNull(databaseId,
firestoreBeamOptions.getFirestoreDb()))
Review Comment:
this will make the IT test always run against a database 'firestoredb' and
you will not be able to override the database name from command line argument
(e.g. --firestoreDb='myTestDb' which could be convenient when you run the test
against your test database), you shouldn't need to hard code the IT test to set
to 'firestoredb', it's configured in the gradle file, e.g.:
https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/build.gradle#L194
##########
CHANGES.md:
##########
@@ -87,7 +87,7 @@
## Bugfixes
-* Fixed X (Java/Python) ([#X](https://github.com/apache/beam/issues/X)).
+* Fixed #36895 (Java) ([#36895](https://github.com/apache/beam/issues/36895)).
Review Comment:
can you add a description of what this fix / issue is? there are examples in
this same file down below, e.g.:
* (Python) Fix WriteToBigQuery transform using CopyJob does not work with
WRITE_TRUNCATE write disposition
([#34247](https://github.com/apache/beam/issues/34247))
--
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]
