Multi-mercury server #12337

samsondav · 2024-03-07T15:04:49Z

No description provided.

github-actions · 2024-03-07T15:05:11Z

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

core/services/relay/evm/mercury/persistence_manager.go

core/services/relay/evm/mercury/transmitter.go

patrickhuie19 · 2024-03-13T17:09:22Z

core/services/ocr2/plugins/mercury/config/config.go

+	return
+}
+
+func ValidatePluginConfig(config PluginConfig, feedID mercuryutils.FeedID) (merr error) {


Should we protect against the same Server URL being specified multiple times? I don't think there are any technical sharp edges with allowing it (Checkout from the mercury pool will make sure a wsrpc client only has one connection to the mercury server IFIUC)

It unmarshals into a map so duplicates would be handled however the TOML library usually handles that. Either the dupe would be ignored, or it would throw an error. Either way its impossible to have dupes after unmarshalling.

patrickhuie19

One non-blocking question; LGTM

core/store/migrate/migrations/0227_add_server_url_to_transmit_requests.sql

cl-sonarqube-production · 2024-03-14T14:11:44Z

Quality Gate passed

Issues
1 New issue
1 Fixed issue
0 Accepted issues

Measures
0 Security Hotspots
87.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

krehermann · 2024-03-14T14:14:40Z

core/services/relay/evm/mercury/persistence_manager_test.go

@@ -23,7 +23,7 @@ func bootstrapPersistenceManager(t *testing.T, jobID int32, db *sqlx.DB) (*Persi
 	t.Helper()
 	lggr, observedLogs := logger.TestLoggerObserved(t, zapcore.DebugLevel)
 	orm := NewORM(db, lggr, pgtest.NewQConfig(true))
-	return NewPersistenceManager(lggr, orm, jobID, 2, 5*time.Millisecond, 5*time.Millisecond), observedLogs
+	return NewPersistenceManager(lggr, "mercuryserver.example", orm, jobID, 2, 5*time.Millisecond, 5*time.Millisecond), observedLogs


is mercuryserver.example a valid wss url? if not, why does this work? i some validation code early in the PR to ensure wss urls. where does that come and where is it tested?

In DB and internally the scheme is always stripped. You can provide URLs in the TOML in either form:

wss://example.com

example.com

Are both valid.

krehermann · 2024-03-14T14:18:11Z

core/services/relay/evm/mercury/persistence_manager.go

@@ -37,10 +38,11 @@ type PersistenceManager struct {
 	pruneFrequency        time.Duration
 }

-func NewPersistenceManager(lggr logger.Logger, orm ORM, jobID int32, maxTransmitQueueSize int, flushDeletesFrequency, pruneFrequency time.Duration) *PersistenceManager {
+func NewPersistenceManager(lggr logger.Logger, serverURL string, orm ORM, jobID int32, maxTransmitQueueSize int, flushDeletesFrequency, pruneFrequency time.Duration) *PersistenceManager {


are persistence manager completely independent? if a mercury reporting plugin writes to two servers and one of those refuses some data or otherwise fails, does the reporting plugin care? asked differently are there any implicit or explicit assumptions when reading from the persistence manager that the target servers are in sync or otherwise have coupled contraints?

Yes they are independent, each PersistenceManager is scoped to a unique URL

if a mercury reporting plugin writes to two servers and one of those refuses some data or otherwise fails, does the reporting plugin care?

Nope, reporting plugin doesn't know or care.

are there any implicit or explicit assumptions when reading from the persistence manager that the target servers are in sync or otherwise have coupled contraints

Nope the queues operate completely independently, by design.

krehermann · 2024-03-14T14:19:03Z

core/services/relay/evm/mercury/queue.go

 	}
 }

+func (tq *TransmitQueue) Init(transmissions []*Transmission) {


should this be wrapped in a do once?

Hmm, no I don't think so. It's up to the caller. Presumably they could Init twice if they wanted.

i only see this called in tests. Is it missing from somewhere else? should there be a guard somewhere that fails cleanly if init hasn't been called? right now i'm concerned there is a nil pointer panic lurking when someone forgets to init

samsondav had a problem deploying to sdlc March 7, 2024 15:04 — with GitHub Actions Failure

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from 9fcfd85 to cddf1a6 Compare March 7, 2024 17:56

samsondav temporarily deployed to sdlc March 7, 2024 17:56 — with GitHub Actions Inactive

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from cddf1a6 to 48e620c Compare March 8, 2024 17:14

samsondav temporarily deployed to sdlc March 8, 2024 17:14 — with GitHub Actions Inactive

samsondav temporarily deployed to sdlc March 8, 2024 17:45 — with GitHub Actions Inactive

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from cd3c53c to e19b233 Compare March 11, 2024 15:54

samsondav temporarily deployed to sdlc March 11, 2024 15:54 — with GitHub Actions Inactive

samsondav marked this pull request as ready for review March 11, 2024 17:55

samsondav requested a review from a team as a code owner March 11, 2024 17:55

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from e19b233 to eb26b48 Compare March 11, 2024 17:56

samsondav temporarily deployed to sdlc March 11, 2024 17:56 — with GitHub Actions Inactive

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from eb26b48 to 67eab55 Compare March 11, 2024 17:57

samsondav temporarily deployed to sdlc March 11, 2024 17:57 — with GitHub Actions Inactive

samsondav temporarily deployed to sdlc March 11, 2024 18:04 — with GitHub Actions Inactive

brunotm reviewed Mar 12, 2024

View reviewed changes

core/services/relay/evm/mercury/persistence_manager.go Outdated Show resolved Hide resolved

brunotm reviewed Mar 12, 2024

View reviewed changes

core/services/relay/evm/mercury/transmitter.go Show resolved Hide resolved

core/services/relay/evm/mercury/transmitter.go Show resolved Hide resolved

samsondav temporarily deployed to sdlc March 12, 2024 12:45 — with GitHub Actions Inactive

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from c12a57e to b9c92ba Compare March 12, 2024 12:46

samsondav temporarily deployed to sdlc March 12, 2024 12:46 — with GitHub Actions Inactive

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from b9c92ba to 6811852 Compare March 12, 2024 12:47

samsondav temporarily deployed to sdlc March 12, 2024 12:47 — with GitHub Actions Inactive

samsondav requested review from brunotm, JoshC2k and akuzni2 March 12, 2024 12:51

samsondav enabled auto-merge March 12, 2024 14:08

samsondav requested a review from jmank88 March 12, 2024 14:08

patrickhuie19 reviewed Mar 13, 2024

View reviewed changes

patrickhuie19 previously approved these changes Mar 13, 2024

View reviewed changes

samsondav added this pull request to the merge queue Mar 13, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 13, 2024

akuzni2 reviewed Mar 13, 2024

View reviewed changes

core/store/migrate/migrations/0227_add_server_url_to_transmit_requests.sql Outdated Show resolved Hide resolved

samsondav added this pull request to the merge queue Mar 14, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 14, 2024

samsondav added 4 commits March 14, 2024 09:57

Multi-mercury server

3d9de4b

Add changeset

12c35c0

Fix race

e862d3d

Bump migration version

5ae5c8c

samsondav dismissed patrickhuie19’s stale review via 5ae5c8c March 14, 2024 13:59

samsondav force-pushed the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch from 6811852 to 5ae5c8c Compare March 14, 2024 13:59

samsondav temporarily deployed to sdlc March 14, 2024 13:59 — with GitHub Actions Inactive

samsondav requested review from akuzni2, patrickhuie19 and krehermann March 14, 2024 13:59

samsondav enabled auto-merge March 14, 2024 14:02

akuzni2 approved these changes Mar 14, 2024

View reviewed changes

krehermann reviewed Mar 14, 2024

View reviewed changes

samsondav added this pull request to the merge queue Mar 14, 2024

krehermann reviewed Mar 14, 2024

View reviewed changes

Merged via the queue into develop with commit 195b504 Mar 14, 2024
97 checks passed

samsondav deleted the MERC-4006-implement-core-node-sending-to-multiple-wsrpc-instances branch March 14, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-mercury server #12337

Multi-mercury server #12337

samsondav commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

patrickhuie19 Mar 13, 2024

samsondav Mar 14, 2024

patrickhuie19 left a comment

cl-sonarqube-production bot commented Mar 14, 2024

krehermann Mar 14, 2024

samsondav Mar 14, 2024

krehermann Mar 14, 2024

samsondav Mar 14, 2024

samsondav Mar 14, 2024

krehermann Mar 14, 2024

samsondav Mar 14, 2024

krehermann Mar 14, 2024

Multi-mercury server #12337

Multi-mercury server #12337

Conversation

samsondav commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhuie19 left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Mar 14, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment