-
Notifications
You must be signed in to change notification settings - Fork 418
Fix: kafka-connect connectors deployment in serial mode #2161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix: kafka-connect connectors deployment in serial mode #2161
Conversation
Removes subgroup handling completely. Only handles single kafka_connect cluster. Fixing defect when rbac mode is enabled and hosts.yml connect cluster hosts are child grouping.
And ensure that it is only run once per cluster in serial mode as well
|
This PR is currently lacking kafka-connect subgroup handling. |
| --- | ||
| - name: Register Kafka Connect Subgroups | ||
| set_fact: | ||
| subgroups: "{{ ((subgroups | default([])) + hostvars[item].group_names) | difference('kafka_broker, ksql, kafka_connect, kafka_rest, kerberos, ksql, schema_registry, kafka_broker_parallel, ksql_parallel, kafka_connect_parallel, kafka_rest_parallel, kerberos_parallel, ksql_parallel, schema_registry_parallel, kafka_controller, kafka_controller_parallel') }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: we have investigated and this seems to be not working as it is expected to work. The difference() function expects an (array, array) as input, but here it consumes (array, string). It doesn't fail, because it implicitly converts str to array of characters.
| vars: | ||
| - connect_cluster_workers: [] | ||
| set_fact: | ||
| active_kafka_connect_groups: "{{ (((active_kafka_connect_groups | default([])) + hostvars[item].group_names) | difference('kafka_connect'+'kafka_connect_parallel'+'kafka_connect_serial'+ 'kafka_broker'+ 'kafka_broker_parallel'+'kafka_broker_serial'+'ksql'+ 'ksql_parallel'+'ksql_serial'+'control_center_next_gen'+'control_center_next_gen_parallel'+'control_center_next_gen_serial'+'schema_registry'+'kafka_rest'+'kafka_rest_parallel'+'kafka_rest_serial'+'kafka_controller'+'kafka_controller_parallel'+'kafka_controller_serial')) | default(['kafka_connect'], true) }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: original code has same issue as explained in https://github.com/confluentinc/cp-ansible/pull/2161/files#r2185136443 .
| parent_kafka_connect_cluster_group: "{{ (group_names | difference(keywords))[0] | default('kafka_connect') }}" | ||
| parent_kafka_connect_cluster_id: "{{ kafka_connect_final_properties['group.id'] }}" | ||
|
|
||
| # TODO needs to be ran over kafka-connect subgroups (i.e. multiple kafka-connect clusters in same inventory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to discuss with someone about how to implement this subgroup in this or higher level.
| - inventory_hostname == ansible_play_hosts_all[0] # for serial mode playbook | ||
| run_once: true | ||
|
|
||
| # TODO needs to be ran over kafka-connect subgroups (i.e. multiple kafka-connect clusters in same inventory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to discuss with someone about how to implement this subgroup in this or higher level.
Description
Fixes #2160
This PR also tries to simplify how role
kafka-connectsregister_clusteranddeploy_connectorsare ran and configured in case where inventory contains multiple kafka-connect clusters.Type of change
How Has This Been Tested?
Currently only in our test environment, but I would be keen to create a test case for this so that we can create a regression test case to this.
Checklist: