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

Wrapped MultiNode client #1006

Merged
merged 8 commits into from
Jan 15, 2025
Merged

Conversation

dhaidashenko
Copy link
Contributor

@dhaidashenko dhaidashenko commented Jan 10, 2025

Wrapped over MultiNode and lazy-loaded client to ensure components like LogPoller can interact with them without need to handle error returned on RPC selection.
See comment.

DylanTinianov
DylanTinianov previously approved these changes Jan 13, 2025
@DylanTinianov DylanTinianov self-requested a review January 13, 2025 16:20
DylanTinianov
DylanTinianov previously approved these changes Jan 13, 2025
@@ -291,6 +293,9 @@ func newChain(id string, cfg *config.TOMLConfig, ks core.Keystore, lggr logger.L

ch.multiNode = multiNode
ch.txSender = txSender
ch.multiClient = client.NewMultiClient(func() (client.ReaderWriter, error) {
return ch.multiNode.SelectRPC()
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this redundant with the prior initialization?

ch.multiClient = client.NewMultiClient(ch.getClient)

Either way, if the multinode is enabled the same SelectRPC() function will be called, right?

// getClient returns a client, randomly selecting one from available and valid nodes
// If multinode is enabled, it will return a client using the multinode selection instead.
func (c *chain) getClient() (client.ReaderWriter, error) {
	if c.cfg.MultiNode.Enabled() {
		return c.multiNode.SelectRPC()
	}

Copy link
Contributor

@reductionista reductionista Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I just re-read @DylanTinianov's comment on my PR and saw he's proposing to get rid of the MultiNode path from c.getClient. If we do that, then I think this makes sense as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could either do that in this PR, or in my PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed second initialization of multiClient.
We can't remove the MultiNode path from c.getClient as it's by other methods of the chain struct.

reductionista
reductionista previously approved these changes Jan 13, 2025
@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
4.5% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

@jadepark-dev jadepark-dev merged commit 0cd58cc into develop Jan 15, 2025
35 of 36 checks passed
@jadepark-dev jadepark-dev deleted the feature/NONEVM-1143-wrapped-multinode branch January 15, 2025 22:19
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