Skip to content
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

feat: mediator role #980

Conversation

niall-shaw
Copy link
Contributor

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]

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 niall-shaw requested a review from a team as a code owner August 10, 2022 14:16
@niall-shaw
Copy link
Contributor Author

Aims to fix #899

@TimoGlastra
Copy link
Contributor

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.

@jakubkoci
Copy link
Contributor

jakubkoci commented Aug 10, 2022

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.

samples/mediator.ts Outdated Show resolved Hide resolved
packages/core/src/agent/Agent.ts Outdated Show resolved Hide resolved
@genaris
Copy link
Contributor

genaris commented Aug 10, 2022

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?

@TimoGlastra
Copy link
Contributor

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

always initialize mediator

Signed-off-by: Niall Shaw <[email protected]>
@niall-shaw niall-shaw changed the title feat: agent mediation role feat: always initialize mediator Aug 18, 2022
@niall-shaw niall-shaw force-pushed the feat/agent-mediation-role branch from 5f80dbd to 092f5f7 Compare August 18, 2022 08:00
@sonarqubecloud
Copy link

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

@niall-shaw niall-shaw closed this Aug 18, 2022
@niall-shaw niall-shaw deleted the feat/agent-mediation-role branch August 18, 2022 08:03
@niall-shaw
Copy link
Contributor Author

Moved to #985

@niall-shaw niall-shaw changed the title feat: always initialize mediator feat: mediator role Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants