Abacn commented on code in PR #29127:
URL: https://github.com/apache/beam/pull/29127#discussion_r1376606582
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/common/GcpIoPipelineOptionsRegistrar.java:
##########
@@ -38,6 +39,7 @@ public Iterable<Class<? extends PipelineOptions>>
getPipelineOptions() {
.add(PubsubOptions.class)
.add(FirestoreOptions.class)
.add(TestBigQueryOptions.class)
+ .add(BigtableTestOptions.class)
Review Comment:
In each IT class, one can get a TestPipelineOptions instance like this
https://github.com/apache/beam/blob/8a28100c47b15cf04ec76aba2e1d00fd760aeebd/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryIOJsonIT.java#L69
and then one can use testOptions.as(BigtableTestOptions.class) to get
BigtableTestOptions.
Current Bigtable ITs is doing similar things and it should suffice.
https://github.com/apache/beam/blob/ba0259de7f9f2d9e06108de20ef57b16ef8e3cc4/sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/changestreams/it/BigtableChangeStreamIT.java#L80C5-L80C12
> unless instanceId is always defined (by registering it for all ITs), other
tests fail because they don't have instanceId defined but we're passing
instanceId.
That is because "registering it for all ITs". If we do not register it for
all ITs then irrelevant tests won't fail.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableTestOptions.java:
##########
@@ -28,10 +28,4 @@ public interface BigtableTestOptions extends
TestPipelineOptions {
String getInstanceId();
void setInstanceId(String value);
-
- @Description("Project for Bigtable")
Review Comment:
If this goes to main it will affect all users. For example, the name
"InstanceId" sounds generic. In the past we received similar reports concerning
this: https://github.com/apache/beam/pull/27256#issuecomment-1745564522
in fact, other gcp component (e.g. Spanner) also has "instanceId" settings
so could introduce confusion.
If it stay within Test scope one can still register it using another
Registrar class. GcpIoPipelineOptionsRegistrar is just an auto service class.
It loads the options at initialization. Besides it, one can register custom
pipeline option classes within Bigtable test classes. This reduces the scope to
necessary level.
ack for removal of `setBigtableProject`
##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableTestUtils.java:
##########
@@ -144,4 +146,24 @@ private static Cell createCell(ByteString value, long
timestamp, String... label
}
return builder.build();
}
+
+ public static BigtableIO.ReadChangeStream buildTestPipelineInput(
Review Comment:
ack. If there are other options needed then good to promote the scope of
this Util class
--
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]
