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

Add configurable ipc timeout #1100

Merged
merged 14 commits into from
Nov 26, 2020

Conversation

SatpalSandhu61
Copy link
Contributor

(Re-raising previously approved PR#145, after resolving merge conflicts from privacy enhancements.)

Currently, the IPC timeout, when quorum communicates with tessera, is hard-coded to 5 seconds.This can cause issues in some cases, for example when a large number of private transactions are sequentially sent to Tessera. Here is one such issue raised in Slack: https://go-quorum.slack.com/archives/C825QTQ1Z/p1600188900086900

This PR allows the timeout to be modified. The change leverages the fact that the envar PRIVATE_CONFIG can already be set to a config file instead of the actual socket file, e.g.: PRIVATE_CONFIG=qdata/dd1/tx-ipc-config.toml

The toml file should contain contents like:

socket = "tm.ipc"
workdir = "qdata/c1"
responseHeaderTimeout = 5

Note that the following timeouts can be specified:

  • dialTimeout is timeout when connecting to socket (seconds), default = 1 second
  • requestTimeout is timeout for the write to the socket (seconds), default = 5 seconds
  • responseHeaderTimeout is timeout for reading a response from the socket (seconds), default = 5 seconds

Krish1979
Krish1979 previously approved these changes Nov 23, 2020
@jpmsam jpmsam requested review from nmvalera and removed request for trung and jbhurat November 24, 2020 13:27
private/private.go Show resolved Hide resolved
private/private.go Show resolved Hide resolved
private/private.go Show resolved Hide resolved
return ptm
}

func newPrivateTxManager(cfg engine.Config) (PrivateTransactionManager, error) {
Copy link
Contributor

@nmvalera nmvalera Nov 26, 2020

Choose a reason for hiding this comment

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

Why not exporting the function?

Also I think NewPrivateTxManager(cfgPath string) could take the config path as an argument possibly returning an extended error using fmt.Errorf(...) so in the end, MustNewPrivateTxManager(cfgPath string) only handles the panic behavior:

func MustNewPrivateTxManager(cfgPath string) PrivateTransactionManager {
    ptm, err := NewPrivateTxManager(cfgPath string)
    if err != nil {
        panic(err)
    }
    return ptm
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is only used locally, hence there's no reason to export it.
I feel it's cleaner as implemented, splitting the load config from the txn manager creation, so I prefer to leave it as is.

@nmvalera nmvalera merged commit 42ba2c6 into Consensys:master Nov 26, 2020
@SatpalSandhu61 SatpalSandhu61 deleted the add-configurable-ipc-timeout branch November 26, 2020 16:18
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.

None yet

3 participants