Add Java documentation to IcebergIO #32621
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
| @@ -54,7 +54,7 @@ public class IcebergWriteSchemaTransformProvider | |||
| extends TypedSchemaTransformProvider<Configuration> { | |||
|
|
|||
| static final String INPUT_TAG = "input"; | |||
| static final String OUTPUT_TAG = "output"; | |||
| static final String SNAPSHOTS_TAG = "snapshots"; | |||
There was a problem hiding this comment.
This is a breaking change that I'm comfortable with because I doubt anyone is fetching snapshots right now (and if they are, they're using .getSinglePcollection() because this tag name is not discoverable yet). I'd like to take the opportunity to give snapshots a more accurate tag and make room for a potential DLQ in the future
| @@ -69,7 +70,7 @@ public Row toRow() { | |||
| try { | |||
| SchemaRegistry registry = SchemaRegistry.createDefault(); | |||
| CODER = registry.getSchemaCoder(SnapshotInfo.class); | |||
| SCHEMA = registry.getSchema(SnapshotInfo.class).sorted(); | |||
| SCHEMA = registry.getSchema(SnapshotInfo.class).sorted().toSnakeCase(); | |||
There was a problem hiding this comment.
Another breaking change that I'm comfortable with. Better to unify everything to snake case format.
|
assign set of reviewers |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| * <pre>{@code | ||
| * Map<String, Object> config = Map.of( | ||
| * "table", table, | ||
| * "triggering_frequency_seconds", 5, |
There was a problem hiding this comment.
I think these details regarding using "Managed" transform for various sources/sinks such as Iceberg should also be in the Website. May be we can also remove it from here (to avoid duplication) once we have these details in a central place. It feels bit odd to have documentation in a class that we do not want users to use directly.
There was a problem hiding this comment.
Agreed that Managed transforms should have a central place -- there are docs that serve this purpose for Dataflow. There isn't any Beam documentation for using IcebergIO though.
It feels bit odd to have documentation in a class that we do not want users to use directly
The class is publicly available for ppl to use (I think this was a prior mistake on our part, we should've kept it package private).
I was thinking if users discover this class, at least they have some clear documentation that drives them towards using the Managed interface.
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergIO.java
Show resolved
Hide resolved
| * pipeline | ||
| * .apply(Create.of(BEAM_ROWS)) | ||
| * .apply(Managed.write(ICEBERG).withConfig(config)) | ||
| * .get("snapshots") |
There was a problem hiding this comment.
We should use the SNAPSHOTS_TAG constant here ?
There was a problem hiding this comment.
Users would have to access it using IcebergWriteSchemaTransformProvider.SNAPSHOTS_TAG, which might be a little too low-level. I think it's better to insulate users from going that deep
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergIO.java
Outdated
Show resolved
Hide resolved
* add documentation to IcebergIO's java class * add example; trigger ITs * nit
No description provided.