[Draft] Refactor config classes #312
Conversation
| .formatName(sourceFormat) | ||
| .build(); | ||
|
|
||
| List<TargetTable> targetTables = |
There was a problem hiding this comment.
I'm wondering if anyone has ideas on how to make the setup less verbose. The construction of the source and the targets share many of the same properties so it seems like there should be a simpler way of doing this.
There was a problem hiding this comment.
Could the parent class, ExternalTable, be used for this along with a copy constructor. i.e. SourceTable.from(ExternalTable table)?
| public class ExternalTable { | ||
| @NonNull String name; | ||
| @NonNull String formatName; | ||
| @NonNull String basePath; |
There was a problem hiding this comment.
nit: A javadoc comment for this member would clarify its purpose and avoid any confusion.
Suggestion:
basePath is the absolute addressable path of the table, which consists of the scheme, the namespace, and the table directories. For example, the basePath of a table, myTable, in ADLS in a namespace named myNamespace would be abfss://[email protected]/foo/bar/myNamespace/myTable
| @NonNull String name; | ||
| @NonNull String formatName; | ||
| @NonNull String basePath; | ||
| @NonNull String dataPath; |
There was a problem hiding this comment.
Clarification: is dataPath generic and applicable to all table formats or just iceberg? IIUC, the default data path for iceberg tables is data/. It is null for both Hudi and Delta by default. If this is correct, then dataPath could be null?
There was a problem hiding this comment.
Delta lake can also handle absolute paths so the metadata and data do not need to be under the same path. The data path is always set if you read through the constructor code below this.
| @NonNull String name; | ||
| @NonNull String formatName; | ||
| @NonNull String basePath; | ||
| @NonNull String dataPath; |
There was a problem hiding this comment.
The dataPath appears to a property of the TargetTable. The dataPath currently seems to point at the folder in which TargetTable's metadata would be generated. As such the TargetTable's basePath should represent it. Does it have any other significance?
There was a problem hiding this comment.
Do you mean SourceTable here? See my comment above about relative vs absolute path requirements. Only Hudi currently requires data and metadata to be under the same prefix.
There was a problem hiding this comment.
Or do you mean that we only need to set a "target metadata path" in the TargetTable? If we want to go that route, does it make sense for the SourceTable to take in a metadata and data path args to be more specific than the current base and data paths
There was a problem hiding this comment.
Iceberg table properties to control location of files include write.data.path, write.metadata.path, and table location. As such these 3 properties could be added to source table.
However, even though Delta supports absolute path for data files, Delta table properties ref do not seem to provide option to control the location of metadata files. Hudi also requires same table location prefix for both data and metadata, and does not seem to provide options to configure these at table level.
As metadata location and data location do not apply to Hudi and Delta, I am wondering if the 3 properties should be part of the generic SourceTable config. The metadata and data locations could be confusing when the table format does not support it. We could document it clearly to avoid confusion. And that maybe better than creating format specific SourceTables (e.g. IcebergSourceTable).
There was a problem hiding this comment.
I've moved the dataPath to the source only. The targets will have a metadata path only and for Hudi that will be required to be the same as the source dataPath for reads to actually work. For the others, you can technically write these metadata files to some arbitrary location and it should work if the file paths are absolute.
| @Getter | ||
| @EqualsAndHashCode(callSuper = true) | ||
| public class TargetTable extends ExternalTable { | ||
| int metadataRetentionInHours; |
There was a problem hiding this comment.
nit: add a javadoc.
This is the retention period for the generated metadata. The translator will automatically delete any generated version in the TargetTable that is older than this period when it runs.
I wonder if using the duration type would make it easier to configure this field with standard time APIs. That way, we could convert days to hours more easily, or set expiry durations that are suitable for testing.
There was a problem hiding this comment.
Makes sense, I've updated the type and will clean things up once this PR is in better shape
|
|
||
| public class TestTargetTable { | ||
| @Test | ||
| void sanitizePath() { |
There was a problem hiding this comment.
nit: The test in TestSourceTable seems identical to this one.
|
|
||
| @Test | ||
| void errorIfRequiredArgsNotSet() { | ||
| assertThrows(NullPointerException.class, () -> SourceTable.builder().name("name").build()); |
There was a problem hiding this comment.
SourceTable?
Would it be better to create a TestExternalTables to avoid duplicating the tests?
|
|
||
| assertThrows( | ||
| NullPointerException.class, | ||
| () -> SourceTable.builder().basePath("file://bucket/path").build()); |
|
Closing this in favor of a new PR #480 |
What is the purpose of the pull request
Aims to address #297
Brief change log
Map<String, String>of client properties when initializing the source client (can be done for target client's as well if there is need) which may contain other parameters required by a specific client that are not common to multiple formats.PerTableConfiginto aSourceTable,TargetTable, andTableSyncConfigto decouple the sync specifics from the table configurationsHudiSourceConfigis now derived from the client propertiesMap<String, String>Verify this pull request
TODO: test RunSync and other flows