-
Notifications
You must be signed in to change notification settings - Fork 44
Avoid refreshing BftScanConnection too often when less than f scans are failing #1151
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
Conversation
[ci] Signed-off-by: Oriol Muñoz <oriol.munoz@digitalasset.com>
Signed-off-by: Oriol Muñoz <oriol.munoz@digitalasset.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the alternative would be to drop the whole retry mechanism, but the problem then is that it's only refreshed every ~10m (which is a good-enough default otherwise)
| val failed = connections.failed | ||
| val total = connections.totalNumber | ||
| val f = connections.f | ||
| if (connections.failed > connections.f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm should this be f or 2f+1? I thought we want 2f+1 working connections so we can read from them and tolerate f failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually that was stupid, refresh already does the check:
val defaultCallConfig = BftCallConfig.default(connections)
// Most but not all calls will use the default config.
// Fail early if there are not enough Scans for the default config
if (!defaultCallConfig.enoughAvailableScans) {
throw io.grpc.Status.FAILED_PRECONDITION
.withDescription(
s"There are not enough Scans to satisfy f=${connections.f}. Will be retried. State: $newState"
)
.asRuntimeException()
} else {
connections
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix'd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(where also the condition is: connections.size >= targetSuccess, i.e. connections.size >= f +1)
Signed-off-by: Oriol Muñoz <oriol.munoz@digitalasset.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Would it make sense to add a test, something like the existing "periodically refresh the list of scans" in BftScanConnectionTest?
Fixes too many refreshes happening even if BFT guarantees are still ensured (see #1141 (comment))