liveness: lazily calculate the node connectivity inside NodeVitality #144272
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
iskettaneh
force-pushed
the
gossip3
branch
2 times, most recently
from
214d487 to
c7b1f4d
Compare
iskettaneh
changed the title
There was a problem hiding this comment.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @iskettaneh)
pkg/kv/kvserver/liveness/livenesspb/liveness.go line 16 at r1 (raw file):
"github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/rpc" "github.com/cockroachdb/cockroach/pkg/rpc/nodedialer"
Would it be possible to stub out the interfaces required here to avoid the dependence on the rpc and nodedialer pkgs? Usually in pb files, we avoid larger deps.
pkg/kv/kvserver/liveness/livenesspb/liveness.go line 200 at r1 (raw file):
func (ncs *NodeConnectionStatus) IsConnected() bool { if !ncs.calculatedConnected.Load() { // If we need to calculate the connection status, we need to take upgrade
nit: upgrade our read lock.
iskettaneh
force-pushed
the
gossip3
branch
2 times, most recently
from
7cd91ca to
2da4a60
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/liveness/livenesspb/liveness.go line 16 at r1 (raw file):
Previously, kvoli (Austen) wrote…
Would it be possible to stub out the interfaces required here to avoid the dependence on the rpc and nodedialer pkgs? Usually in
pbfiles, we avoid larger deps.
I added a thin interface to remove the dependency on the rpc/nodedialer. Also, while I am at it, I added a test that asserts the caching behaviour (introduced a light mock to do that).
pkg/kv/kvserver/liveness/livenesspb/liveness.go line 200 at r1 (raw file):
Previously, kvoli (Austen) wrote…
nit: upgrade our read lock.
I changed the whole paragraph. I realized that the comment is stale (came in my first iteration of doing this work, where I was using read/write locks).
Before this commit, always calculate if we are connected to the node as part of the NodeVitality creation. However, most uses of node vitality don't need that calculation to be made. This commit addresses this performance issue by lazily calculating the connected field only when requested. Fixes: cockroachdb#143144 Release note: None
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
|
TFTR! bors r+ |
Before this commit, always calculate if we are connected to the node as part of the NodeVitality creation. However, most uses of node vitality don't need that calculation to be made. This commit addresses this performance issue by lazily calculating the connected field only when requested.
Fixes: #143144
Release note: None