Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4363 +/- ##
============================================
+ Coverage 33.89% 38.08% +4.19%
- Complexity 505 517 +12
============================================
Files 215 195 -20
Lines 13186 11853 -1333
Branches 1320 1209 -111
============================================
+ Hits 4469 4514 +45
+ Misses 8350 6968 -1382
- Partials 367 371 +4
🚀 New features to boost your workflow:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Kafka to Bigtable YAML template for Dataflow, facilitating seamless streaming data ingestion. It also performs a cleanup of existing Kafka-based templates by removing several unsupported configuration parameters, ensuring a more stable and consistent configuration interface across the template suite. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the new Kafka to Bigtable (YAML) template, which includes its Java interface, YAML pipeline definition, documentation, and integration tests. Additionally, it cleans up several unused parameters (such as allowDuplicates and redistribution options) from existing Kafka-to-Iceberg and Kafka-to-BigQuery YAML templates. The review feedback highlights several improvement opportunities: managing the KafkaProducer in the integration test with a try-with-resources block to avoid resource leaks, correcting the YAML list syntax for requirements in KafkaToBigtable.yaml, and providing realistic examples instead of empty strings for the language and fields template parameters.
0b215a2 to
1d0c24e
Compare
damccorm
left a comment
There was a problem hiding this comment.
Generally this LGTM. I did have one open question
| required: true | ||
| type: text | ||
| - order: 3 | ||
| name: "allowDuplicates" |
There was a problem hiding this comment.
Why are these options unsupported? Have they just not made it to yaml yet?
There was a problem hiding this comment.
see apache/beam#40396, but they were not fully connected when initially added.
|
Actually, found a few more potential issues with Jetski, I'll add those too. |
| - type: WindowInto | ||
| name: WindowInto | ||
| input: ConvertStringsToBytes | ||
| config: | ||
| windowing: {{ windowing }} | ||
| - type: WriteToBigTable | ||
| name: WriteToBigtable | ||
| input: WindowInto | ||
| config: | ||
| project: {{ projectId }} | ||
| instance: {{ instanceId }} | ||
| table: {{ tableId }} |
There was a problem hiding this comment.
windowing is marked as optional = true in KafkaToBigtableYaml.java (because windowinto_options.yaml has is_optional: false instead of required: true or required: false), but WindowInto is included unconditionally here. If a user omits windowing, main.py strips the undefined windowing key and leaves WindowInto with config: {}, which will fail at pipeline construction.
Since Bigtable writes don't require windowing, should we make WindowInto conditional when windowing is provided (and update windowinto_options.yaml to use required: false)?
| - type: WindowInto | |
| name: WindowInto | |
| input: ConvertStringsToBytes | |
| config: | |
| windowing: {{ windowing }} | |
| - type: WriteToBigTable | |
| name: WriteToBigtable | |
| input: WindowInto | |
| config: | |
| project: {{ projectId }} | |
| instance: {{ instanceId }} | |
| table: {{ tableId }} | |
| {% if windowing %} | |
| - type: WindowInto | |
| name: WindowInto | |
| input: ConvertStringsToBytes | |
| config: | |
| windowing: {{ windowing }} | |
| {% endif %} | |
| - type: WriteToBigTable | |
| name: WriteToBigtable | |
| input: {% if windowing %}WindowInto{% else %}ConvertStringsToBytes{% endif %} | |
| config: | |
| project: {{ projectId }} | |
| instance: {{ instanceId }} | |
| table: {{ tableId }} |
There was a problem hiding this comment.
It should match the existing blueprint and be required. I have updated. Thanks
| - type: MapToFields | ||
| name: ConvertStringsToBytes | ||
| input: ReadFromKafka | ||
| config: | ||
| language: {{ language }} | ||
| fields: {{ fields }} |
There was a problem hiding this comment.
Question: Unlike PubSubToBigtable.yaml and KafkaToBigQuery.yaml, ReadFromKafka and MapToFields here don't configure error_handling with a dead-letter sink. Was omitting a DLQ intentional for the initial blueprint (similar to KafkaToIceberg.yaml), or should we include error handling so a malformed record / failed Python expression doesn't stall the streaming pipeline?
There was a problem hiding this comment.
We should probably include this (and it should probably be included in KafkaToIceberg as well
There was a problem hiding this comment.
Intentional move at this point to match existing. In future we can add DLQ to all if it makes sense to.
There was a problem hiding this comment.
Could we take this as a fast follow then? I think it would be a good feature to have for all streaming blueprints.
There was a problem hiding this comment.
Sure, after all blueprints are migrated and match JB, everything is supported in JB during reimport, JB examples are decommissioned, then I will follow up with DLQ. Sound good? Thanks
| example = "true") | ||
| Boolean getAllowDuplicates(); | ||
|
|
||
| @TemplateParameter.Text( |
There was a problem hiding this comment.
Since this PR cleans up unsupported parameters in kafka_options.yaml and fixes consumer_config: {{ consumerConfigUpdates }} in KafkaToIceberg.yaml, should we also wire confluent_schema_registry_subject, confluent_schema_registry_url, consumer_config, file_descriptor_path, and message_name into ReadFromKafka in KafkaToBigQuery.yaml? Right now KafkaToBigQueryYaml.java exposes those parameters from kafka_read_options, but KafkaToBigQuery.yaml only passes bootstrap_servers, topic, format, and schema.
There was a problem hiding this comment.
I would prefer I open a separate PR to add supported parameters.
There was a problem hiding this comment.
Opened #4397 to track these requests. Thanks
| help: > | ||
| The language used to define (and execute) the expressions and/or | ||
| callables in fields. Defaults to generic. | ||
| example: "python" | ||
| required: true |
There was a problem hiding this comment.
Nit: The help text for language says "Defaults to generic.", but required: true is set without a default. Should this either have default: "generic" (with required: false), or should "Defaults to generic." be removed from the help text?
| order = 12, | ||
| name = "projectId", | ||
| optional = false, | ||
| description = "Bigtable project ID", | ||
| helpText = "The Google Cloud project ID of the Bigtable instance.", | ||
| example = "") | ||
| @Validation.Required | ||
| String getProjectId(); | ||
|
|
||
| @TemplateParameter.Text( | ||
| order = 13, | ||
| name = "instanceId", | ||
| optional = false, | ||
| description = "Bigtable instance ID", | ||
| helpText = "The Bigtable instance ID.", | ||
| example = "") | ||
| @Validation.Required | ||
| String getInstanceId(); | ||
|
|
||
| @TemplateParameter.Text( | ||
| order = 14, | ||
| name = "tableId", | ||
| optional = false, | ||
| description = "Bigtable output table", | ||
| helpText = "Bigtable table ID to write the output to.", | ||
| example = "") | ||
| @Validation.Required | ||
| String getTableId(); | ||
|
|
||
| @TemplateParameter.Text( | ||
| order = 15, | ||
| name = "windowing", | ||
| optional = true, | ||
| description = "Windowing options", | ||
| helpText = | ||
| "Windowing options - see https://beam.apache.org/documentation/sdks/yaml/#windowing", | ||
| example = "") | ||
| String getWindowing(); |
There was a problem hiding this comment.
Nit: projectId, instanceId, tableId (in bigtable_options.yaml) and windowing (in windowinto_options.yaml) all generate example = "". Consider adding example values in those option files as well.
Uh oh!
There was an error while loading. Please reload this page.