PIN-7197 - notification-tenant-lifecycle-consumer #2179
Conversation
tpetrucciani
changed the base branch from
PIN-7226_notification_config_create_delete
to
develop
tpetrucciani
changed the base branch from
develop
to
PIN-7226_notification_config_create_delete
tpetrucciani
force-pushed
the
PIN-7197_notification_tenant_lifecycle_consumer
branch
2 times, most recently
from
b7c3574 to
84972e0
Compare
tpetrucciani
requested review from
beetlecrunch,
galales,
AsterITA,
Carminepo2 and
taglioni-r
as code owners
tpetrucciani
requested review from
borgesis95,
sandrotaje and
MatteoGrilli-quinck
| if ( | ||
| (await tenantReadModelService.getTenantById(tenantId)) === undefined | ||
| ) { | ||
| logger.warn( | ||
| `Tenant ${tenantId} not found, creating default notification configuration anyway (assuming the readmodel is not yet updated with the new tenant)` | ||
| ); | ||
| } |
There was a problem hiding this comment.
We're not sure how to deal with this scenario. We'd like to ensure that the default notification config is only created for existing tenants and only deleted when the tenant is deleted. However, we're creating/deleting it in response to TenantOnboarded/MaintenanceTenantDeleted events, therefore it might occur when the readmodel isn't updated yet.
We decided just to log a warning in case the process doesn't see the tenant in the readmodel yet (and conversely, in the delete, if it still sees it).
This could help notice hypotetical issues in which we had spurious calls to these APIs, but, in practice, we expect these logs to occur just because this code is executed before the readmodel is updated, in which case it's OK (since anyway a TenantOnboarded event has occurred). So I'm not sure how useful this is.
We could have the creation or deletion fail outright in these cases, but then we'd force the notification-tenant-lifecycle-consumer to retry until it succeeds, which is maybe wasteful (and produces spurious error logs) since it's not technically an issue to create the config before the tenant.
There was a problem hiding this comment.
I think that making the request succeed without performing the insertion could bring some extra activities, like triggering the endpoint again manually after the readmodel gets updated.
It might be easier to make the request fail, so that the consumer calling it will retry until it crashes. At that point we would be already aware to double-check if the tenant has been finally written in the readmodel and then restart the consumer without any other activity.
What do you think? We could just make an evaluation in dev to see how much this scenario is likely to happen.
There was a problem hiding this comment.
Sorry, I misunderstood this:
We decided just to log a warning in case the process [...]
So the notification config gets actually written, even though the tenant is not in readmodel yet.
There was a problem hiding this comment.
The only bad scenario would be a tenant retrieving its own notification config before the tenant itself is written in readmodel. If we avoid that read operation, it should be ok.
There was a problem hiding this comment.
The only bad scenario would be a tenant retrieving its own notification config before the tenant itself is written in readmodel. If we avoid that read operation, it should be ok.
That's right. I suppose that, if the tenant isn't in the readmodel yet, the user would be unable to access the frontend or receive other errors anyway (e.g. because the frontend can't fetch the user's tenant to display it?).
The other hypothetical risk is creating spurious configs for tenants that don't exist if we somehow call the route with a non-existent id. But I think it's an unlikely bug and also, while creating some spurious configs that won't be used, doesn't have actual impacts.
For the delete, if by mistake it gets called for an existing tenant, we'd delete the configuration and, afterwards, that tenant wouldn't be notified (and users in that tenant would probably get an error if they try to access their notification configurations). But again I think it's an unlikely issue that we end up calling the route with a wrong ID that matches another tenant.
| await match(decodedMessage) | ||
| .with({ event_version: 1 }, () => { | ||
| loggerInstance.info( | ||
| `Discarding message with event_version = 1. Partition number: ${partition}. Offset: ${message.offset}` | ||
| ); | ||
| }) | ||
| .with({ event_version: 2 }, (msg) => { | ||
| loggerInstance.info( | ||
| `Processing ${decodedMessage.type} message - Partition number: ${partition}. Offset: ${message.offset}` |
There was a problem hiding this comment.
We thought to handle only V2 tenant events, not V1, since it's a new service. For past data, we suppose we'll have a custom process anyway, without using the consumer, to avoid re-running all tenant events.
Does this make sense to you? Asking @galales too for confirmation.
There was a problem hiding this comment.
I think that handling here all the events (past v1, past v2, incoming v2) would simplify the reprocess scenario, but up to @galales
tpetrucciani
force-pushed
the
PIN-7197_notification_tenant_lifecycle_consumer
branch
from
9783c0c to
f67d341
Compare
| { notificationConfigProcess }: PagoPAInteropBeClients | ||
| ) { | ||
| return { | ||
| async processMessage( |
There was a problem hiding this comment.
| async processMessage( | |
| async handleMessage( |
I would rename this in order to be different from the one in index.ts
This PR adds a new consumer that listens to tenant onboarding and deletion events and calls the notification-config-process APIs to create a default notification config for the new tenant or delete the notification config when the tenant is deleted