nsivabalan commented on code in PR #9482:
URL: https://github.com/apache/hudi/pull/9482#discussion_r1311022781
##########
hudi-gcp/src/main/java/org/apache/hudi/gcp/bigquery/BigQuerySyncTool.java:
##########
@@ -52,34 +57,55 @@ public class BigQuerySyncTool extends HoodieSyncTool {
private static final Logger LOG =
LoggerFactory.getLogger(BigQuerySyncTool.class);
- public final BigQuerySyncConfig config;
- public final String tableName;
- public final String manifestTableName;
- public final String versionsTableName;
- public final String snapshotViewName;
+ private final BigQuerySyncConfig config;
+ private final String tableName;
+ private final String manifestTableName;
+ private final String versionsTableName;
+ private final String snapshotViewName;
+ private final ManifestFileWriter manifestFileWriter;
+ private final HoodieBigQuerySyncClient bqSyncClient;
+ private final HoodieTableMetaClient metaClient;
+ private final BigQuerySchemaResolver bqSchemaResolver;
public BigQuerySyncTool(Properties props) {
- super(props);
+ // will build file writer, client, etc. from configs
+ this(props, null, null, null, null);
Review Comment:
can we do the initialization here only and avoid
```
this.bqSyncClient = providedBqSyncClient == null ? new
HoodieBigQuerySyncClient(config) : providedBqSyncClient;
```
calls like this in the other constructor.
##########
hudi-gcp/src/main/java/org/apache/hudi/gcp/bigquery/BigQuerySyncTool.java:
##########
@@ -52,34 +57,55 @@ public class BigQuerySyncTool extends HoodieSyncTool {
private static final Logger LOG =
LoggerFactory.getLogger(BigQuerySyncTool.class);
- public final BigQuerySyncConfig config;
- public final String tableName;
- public final String manifestTableName;
- public final String versionsTableName;
- public final String snapshotViewName;
+ private final BigQuerySyncConfig config;
+ private final String tableName;
+ private final String manifestTableName;
+ private final String versionsTableName;
+ private final String snapshotViewName;
+ private final ManifestFileWriter manifestFileWriter;
+ private final HoodieBigQuerySyncClient bqSyncClient;
+ private final HoodieTableMetaClient metaClient;
+ private final BigQuerySchemaResolver bqSchemaResolver;
public BigQuerySyncTool(Properties props) {
- super(props);
+ // will build file writer, client, etc. from configs
+ this(props, null, null, null, null);
+ }
+
+ @VisibleForTesting // allows us to pass in mocks for the writer and client
+ BigQuerySyncTool(Properties properties, ManifestFileWriter
manifestFileWriter, HoodieBigQuerySyncClient providedBqSyncClient,
HoodieTableMetaClient providedMetaClient,
+ BigQuerySchemaResolver providedBqSchemaResolver) {
+ super(properties);
this.config = new BigQuerySyncConfig(props);
this.tableName = config.getString(BIGQUERY_SYNC_TABLE_NAME);
this.manifestTableName = tableName + "_manifest";
this.versionsTableName = tableName + "_versions";
this.snapshotViewName = tableName;
+ this.bqSyncClient = providedBqSyncClient == null ? new
HoodieBigQuerySyncClient(config) : providedBqSyncClient;
+ // reuse existing meta client if not provided (only test cases will
provide their own meta client)
Review Comment:
lets not block this PR on this discussion. we can take it async. lets go
ahead w/ the patch. I am good
--
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]
