go/grpc: sentry nodes #2475
go/grpc: sentry nodes #2475
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2475 +/- ##
==========================================
- Coverage 63.74% 63.32% -0.43%
==========================================
Files 346 354 +8
Lines 32672 33433 +761
==========================================
+ Hits 20828 21170 +342
- Misses 9265 9633 +368
- Partials 2579 2630 +51
Continue to review full report at Codecov.
|
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
13 times, most recently
from
23366e4 to
a13e086
Compare
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
5 times, most recently
from
4b7ee1a to
2ee99c4
Compare
ptrus
added
c:breaking/consensus
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
from
216437b to
573a41e
Compare
go/worker/sentry/grpc/worker.go
Outdated
| case p, ok := <-pCh: | ||
| if !ok { | ||
| g.logger.Error("WatchPolicies stream failure") | ||
| // TODO: Panic? Retry? |
There was a problem hiding this comment.
i think connection errors with the upstream (for whatever reason) would make this fail
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
2 times, most recently
from
0cd571c to
4db97ee
Compare
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
5 times, most recently
from
077a285 to
22154ba
Compare
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
4 times, most recently
from
080d80f to
6b251ae
Compare
|
|
||
| // Handler returns a grpc StreamHandler than can be used | ||
| // to proxy requests to provided client. | ||
| // XXX: potentially the connection should be established in this package, |
There was a problem hiding this comment.
Plan to file issues around these?
There was a problem hiding this comment.
yeah i'll file a single issue about proxy benchmarks and potential improvements, since i feel like for most of these some benchmarks should be first done (e.g. compare perf of accessing a storage node directly or via a sentry)
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
3 times, most recently
from
f3fb821 to
33bec30
Compare
go/oasis-test-runner/oasis/args.go
Outdated
| func (args *argBuilder) workerRegistrySentryAddresses(addrs []string) *argBuilder { | ||
| func (args *argBuilder) disableRegistrationWorker() *argBuilder { | ||
| args.vec = append(args.vec, []string{ | ||
| "--" + registration.CfgRegistrationWorkerEnabled + "=false", |
There was a problem hiding this comment.
Is this a new flag? It is not mentioned in the changelog fragment. Also, is this required because of #1930 (e.g., it could be handled automatically)?
There was a problem hiding this comment.
Oh forgot to add it to the changelog fragment. So it's not technically needed actually, i just wanted to disable the registration worker on sentry nodes as it's not needed (in case it's enabled the registrations are failing anyway, so no actual difference). So i'm fine with removing the flag and postponing this to the #1930 - i don't see a reason it couldn't be done automatically (maybe by checking node roles?).
There was a problem hiding this comment.
i removed it for now as it's not technically needed, and it can be properly fixed with #1930
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
from
33bec30 to
69561fe
Compare
Actually with gRPC sentries, the sentry worker flags are now also used out of registration worker so having those flags in common worker makes more sense. Not sure how i managed to miss that when doing the renaming in previous PR.
ptrus
force-pushed
the
ptrus/feature/grpc-sentry-support
branch
from
69561fe to
1eea851
Compare
|
thanks for all the reviews on the huge PR @kostko :-) |
Closes: #1829
TODO: