Check disabled connections after protocol update #18990
Conversation
gosusnp
requested review from
colesnodgrass,
jdpgrailsdev,
benmoriceau and
alovew
github-actions
bot
added
area/platform
|
So for connections that have unsupported_protocol_version=true, we are not updating the status to inactive=true? we just rely on that field to determine whether it is disabled or not? |
@alovew, yes, the desired behavior is that after a connector update, if it fixes some protocol version conflicts, we'd automatically restore the connection to the state they were in before the conflict. We opted for adding a flag to mark the issue rather than changing the actual status. It means that we need to update how we lookup connections to take this into consideration. This will be part of the work to actual disable connections which is going to be my next task. |
| .select(CONNECTION.ID, sourceDef.ID, sourceDef.PROTOCOL_VERSION, destDef.ID, destDef.PROTOCOL_VERSION) | ||
| .from(CONNECTION) | ||
| .join(source).on(CONNECTION.SOURCE_ID.eq(source.ID)) | ||
| .join(sourceDef).on(source.ACTOR_DEFINITION_ID.eq(sourceDef.ID)) |
There was a problem hiding this comment.
Why do we need the join on the source / source def ? I think that it is missing in the where clause.
There was a problem hiding this comment.
To keep things simple, my approach here was to always retrieve (ConnectionId, sourceDefId, sourceProtocolVersion, destDefId, destProtocolVersion).
Since protocol version is store in the actor definition, we always need to join both sides (source+destination).
An alternative could be to have some specific logic to only filter source or destination, however, I think it adds a few more conditional blocks overall throughout the code base for an operation that in the end should be relatively rare.
| .join(destDef).on(destination.ACTOR_DEFINITION_ID.eq(destDef.ID)) | ||
| .where( | ||
| CONNECTION.UNSUPPORTED_PROTOCOL_VERSION.eq(true).and( | ||
| (actorType == ActorType.DESTINATION ? destDef : sourceDef).ID.eq(actorDefId))) |
There was a problem hiding this comment.
Should we do the same for the Source Actor type?
There was a problem hiding this comment.
This is what the inline if is doing here, based on the actorType it will either add where source.ID.eq(actorDefId) or destination.ID.eq(actorDefId)
| } | ||
|
|
||
| private void clearProtocolVersionFlag(final DSLContext ctx, final List<UUID> standardSyncIds) { | ||
| ctx.update(CONNECTION) |
There was a problem hiding this comment.
Nit: Please update the modified_at collumn as well. I don't think that is being updated through a trigger.
There was a problem hiding this comment.
Good catch, updated.
…fter-protocol-update
gosusnp
deleted the
gosusnp/16043-check-disabled-connections-after-protocol-update
branch
…nent * master: 🪟 🎉 Enable frontend validation for <1hr syncs (cloud) #19028 Stream returns AirbyteMessages (#18572) 🎉 New Source - Recruitee [low-code SDK] (#18671) 🎉 New source: Breezometer [low-code cdk] (#18650) Check disabled connections after protocol update (#18990) Simple default replication worker refactor (#19002) 🎉 New Source: Visma e-conomic (#18595) 🎉 New Source: Fastbill (#18593) Bmoric/extract state api (#18980) 🎉 New Source: Zapier Supported Storage (#18442) 🎉 New source: Klarna (#18385) `AirbyteEstimateTraceMessage` (#18875) Extract source definition api (#18977) [low-code cdk] Allow for spec file to be defined in the yaml manifest instead of an external file (#18411) 🐛 Source HubSpot: fix property scopes (#18624) Ensure that only 6-character hex values are passed to monaco editor (#18943)
What
Following a connector update, we want to re-enable connections that may have been disabled due to an unsupported protocol version if possible.
Closes #16043
How
Upon successful a connector update, clear unsupported protocol version flags if the protocol version is back within support range.
Recommended reading order
StandardSyncPersistence.javacontains the actual logic to re-enable connection🚨 User Impact 🚨
No breaking change, we can start to re-enable connection that would have been disabled due to protocol version, however, the disable step doesn't exist yet so no impact here.