CUMULUS-4788: Move replication for different tables into separate modules#4403
CUMULUS-4788: Move replication for different tables into separate modules#4403indiejames merged 6 commits intomasterfrom
Conversation
indiejames
force-pushed
the
cumulus-4788-split-replication-service
branch
from
44f6d0c to
257d138
Compare
chris-durbin
left a comment
chris-durbin
left a comment
There was a problem hiding this comment.
There are a lot of variables that need to be set up - do we have a tfvars example template that could be filled in similar to other cumulus deployments? I'm also not sure of the mechanics for figuring out and setting all of the top level deployment variables.
| } | ||
|
|
||
| variable "ecs_infrastructure_role" { | ||
| type = object({ |
There was a problem hiding this comment.
Add descriptions for these 4 variables
|
|
||
| locals { | ||
| full_name = "${var.prefix}-replication" | ||
| # full_name = "${var.prefix}-replication" |
| name = "${var.prefix}-CumulusIcebergReplicationECSCluster" | ||
| tags = var.tags | ||
| } | ||
| # resource "aws_security_group" "no_ingress_all_egress" { |
| description = "Subnets for database cluster. Requires at least 2 across multiple AZs" | ||
| type = list(string) | ||
| variable "subnet" { | ||
| description = "Subnet for Fargate tasks" |
There was a problem hiding this comment.
It's probably worth a note on why you are using a single subnet (I think you said an EBS limitation).
| type = object({ bucket = string, key = string, region = string }) | ||
| } | ||
|
|
||
| variable "iceberg_s3_bucket" { |
There was a problem hiding this comment.
It looks like we aren't creating these buckets in Terraform - is the plan to create them via the script that sets up the Iceberg tables?
There was a problem hiding this comment.
I thought that was what you said, but we can create them in the terraform if you want.
There was a problem hiding this comment.
Ok I think a terraform variable here and created by the loading script is probably the most flexible - we can always change our minds later.
| kafka_image = var.kafka_image | ||
| connect_image = var.connect_image | ||
| bootstrap_image = var.bootstrap_image | ||
| pg_db = "postgres" |
There was a problem hiding this comment.
I think the db name should be coming from an output of the RDS module.
There was a problem hiding this comment.
I don't think it can? The terraform is not setting up the dB, it it? That's some script running migrations, I tihnk
There was a problem hiding this comment.
In that case we should make it a variable to be passed in instead of hardcoding to postgres.
| } | ||
|
|
||
| variable "slot_name" { | ||
| description = "The name for the replication slot" |
There was a problem hiding this comment.
This variable needs a better name or at least a better description.
There was a problem hiding this comment.
maybe something like replication_table_group?
| @@ -0,0 +1,19 @@ | |||
| output "task_execution_role" { | |||
There was a problem hiding this comment.
Add descriptions for the outputs.
There already is a tfvars example file in the rds-iceberg-replications-tf. I'll add the new variables for this PR to it. |
column exclude list to replication terraform code
indiejames
force-pushed
the
cumulus-4788-split-replication-service
branch
from
01e060a to
87337a6
Compare
3 participants
Splits ECS replication service into multiple services to prevent resource exhaustion
Changes
PR Checklist
📝 Note:
For most pull requests, please Squash and merge to maintain a clean and readable commit history.