credo-ts icon indicating copy to clipboard operation
credo-ts copied to clipboard

feat: agent mediation role

Open niall-shaw opened this issue 3 years ago • 5 comments

Add mediationRole property to AgentConfig to ensure MediatorModule is initialized correctly. This prevents concurrency issues when multiple attempts to create a mediatorRoutingRecord fail.

Signed-off-by: Niall Shaw [email protected]

niall-shaw avatar Aug 10 '22 14:08 niall-shaw

Aims to fix #899

niall-shaw avatar Aug 10 '22 14:08 niall-shaw

Technically an agent can be both a mediator and a recipient, so I'm not to keen on adding the role property to the config. I see two options, but there's probably other good options:

  • Always initialize it. It adds some overhead, but it's not that much
  • Add an option e.g. initializeMediatorKeysOnStartup that will init the mediator keys.

In addition, I'm not sure if it should be impossible to use mediation without the mediation role . E.g. with the ledger we have connectToLedgersOnStartup. If you have it enabled it will connect on startup, but otherwise when first requested. Is that something we could also do here?

Another issue is that this won't work for high availability deployments (multiple instances connecting to the same database). There's more places where this will lead to issues, so we can probably ignore this for now.

TimoGlastra avatar Aug 10 '22 14:08 TimoGlastra

Technically an agent can be both a mediator and a recipient

Then it might be an array of roles. I usually try to avoid these flag config attributes (you need to understand what it does instead of just saying I want to run this agent as a mediator and don't care how the framework manages that), but if the array is a problem I'm also ok with something like initializeMediatorRoutingOnStartup. The word Keys seem to be too low-level detail.

Another issue is that this won't work for high availability deployments (multiple instances connecting to the same database). There's more places where this will lead to issues, so we can probably ignore this for now.

Yes, without this change, it doesn't work even with a few edge agents trying to connect at the same time, so I would rather do this now and fix "high availability deployments" later.

jakubkoci avatar Aug 10 '22 15:08 jakubkoci

Technically an agent can be both a mediator and a recipient, so I'm not to keen on adding the role property to the config. I see two options, but there's probably other good options:

  • Always initialize it. It adds some overhead, but it's not that much
  • Add an option e.g. initializeMediatorKeysOnStartup that will init the mediator keys.

In addition, I'm not sure if it should be impossible to use mediation without the mediation role . E.g. with the ledger we have connectToLedgersOnStartup. If you have it enabled it will connect on startup, but otherwise when first requested. Is that something we could also do here?

Another issue is that this won't work for high availability deployments (multiple instances connecting to the same database). There's more places where this will lead to issues, so we can probably ignore this for now.

Probably I'm missing something, but I don't see a major problem on always initializing mediator module, as it will actually initialize the routing key only once and then just query it and store it in the internal variable.

In such case, no new config parameters would be needed. What do you think?

genaris avatar Aug 10 '22 15:08 genaris

Agree with @genaris here. I think avoiding the config option is worth the little bit of extra overhead.

TimoGlastra avatar Aug 11 '22 08:08 TimoGlastra

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Aug 18 '22 08:08 sonarqubecloud[bot]

Moved to #985

niall-shaw avatar Aug 18 '22 08:08 niall-shaw