damondouglas commented on code in PR #28288:
URL: https://github.com/apache/beam/pull/28288#discussion_r1330738321
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableIO.java:
##########
@@ -688,14 +688,13 @@ public final String toString() {
private void validateTableExists(
BigtableConfig config, BigtableReadOptions readOptions,
PipelineOptions options) {
if (config.getValidate() && config.isDataAccessible() &&
readOptions.isDataAccessible()) {
- String tableId = checkNotNull(readOptions.getTableId().get());
+ ValueProvider<String> tableIdProvider =
checkArgumentNotNull(readOptions.getTableId());
+ String tableId = checkArgumentNotNull(tableIdProvider.get());
try {
- checkArgument(
- getServiceFactory().checkTableExists(config, options, tableId),
- "Table %s does not exist",
- tableId);
+ boolean exists = getServiceFactory().checkTableExists(config,
options, tableId);
+ checkArgument(exists, "Table %s does not exist", tableId);
} catch (IOException e) {
- LOG.warn("Error checking whether table {} exists; proceeding.",
tableId, e);
+ throw new IllegalArgumentException(e);
Review Comment:
@mutianf Thank you again for your review and comments. Given the following,
do you feel comfortable with the code changes?
1) We cannot throw an IOException as the overridden `public void
validate(PipelineOptions options)` method cannot also throw this IOException.
Therefore, we have to use a subclass of the RuntimeException, hence the use of
IllegalArgumentException. The other choice would just be a RuntimeException.
However, it seems to make sense that the arguments provided led to the
Exception thrown.
2) The BigtableServiceFactory's checkTableExists already provides detail on
the tableId and the originating ApiException. Therefore, the throw new
IllegalArgumentException(e) would capture these details already.
In summary, I see the following scenarios:
| Scenario | Exception Message | Code Origin |
| --- | --- | --- |
| Not Found ApiException | Table <> does not exist |
[BigtableServiceFactory.java:206](https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceFactory.java#L206)
|
| ApiException | "Error checking whether table <> exists" with ApiException
details |
[BigtableServiceFactory.java:210](https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableServiceFactory.java#L210)
|
--
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]
