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

liveness: introduce package for node liveness #56221

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

irfansharif
Copy link
Contributor

@irfansharif irfansharif commented Nov 3, 2020

NodeLiveness is a relatively isolated component and makes for a good
"should be a standalone package" candidate. We tease it out here, mostly
through mechanical code movements, in the hope that it'll force us to
clarify API boundaries going forward. Some intended changes to these
APIs, like introducing a way to read liveness records directly from KV,
will be added to this new package in other PRs. (These APIs were
originally proposed in #56107, and will be pulled out into stand alone
PRs after this one.)

While here, we also move out the liveness proto definition into this new
package. The "changes" made in this PR are the following:

  pkg/kv/kvserver/{node liveness files}               => pkg/kv/kvserver/liveness
  pkg/kv/kvserver/liveness/{node liveness protos}     => pkg/kv/kvserver/liveness/livenesspb
  pkg/kv/kvserver/liveness/node_liveness_test.go      => pkg/kv/kvserver/liveness/client_test.go
  pkg/kv/kvserver/liveness/node_liveness_unit_test.go => pkg/kv/kvserver/liveness/liveness_test.go

Separately, we've had to initialize server factory in TestMain as the
package internal tests depend on it. This was previously done within
pkg/kv/kvserver/main_test.go.

We've also renamed a few exported symbols to avoid stuttering. This was
needed to make our linters happy.


I've left the existing node liveness tests that make use of
multiTestContext in the kvserver package, seeing as how we're slowly
upgrading them to use TestCluster instead (#8299). The expectation is
that as we migrate each test, they'll move into this new package.

Release note: None

@irfansharif irfansharif requested review from knz and tbg November 3, 2020 01:01
@irfansharif irfansharif requested a review from a team as a code owner November 3, 2020 01:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@irfansharif
Copy link
Contributor Author

I'm not actually sure if the proto file movement is safe. I think it is, because looks like we did the same thing earlier when moving from pkg/storage to pkg/kv/..., but not sure if I've missed something.

@irfansharif irfansharif changed the title nodeliveness: introduce new nodeliveness package liveness: introduce new node liveness package Nov 3, 2020
@irfansharif irfansharif force-pushed the 201102.pkg-nodeliveness branch 2 times, most recently from 1c947a3 to 1ea21b7 Compare November 3, 2020 05:03
@irfansharif irfansharif requested review from a team and adityamaru and removed request for a team and adityamaru November 3, 2020 05:03
@irfansharif irfansharif changed the title liveness: introduce new node liveness package liveness: introduce package for node liveness Nov 3, 2020
@tbg
Copy link
Member

tbg commented Nov 3, 2020

I don't think we should move the node liveness code out of the ./pkg/kv package tree at this point. I think your reasoning is that the service liveness provides is used by the "whole cluster", but with the introduction of multi-tenancy, this is no longer true - liveness is really a statement about the liveness of a KV server. Besides, the implementation and API is heavily KV-centric including plenty of internals (special txn triggers, gossip), and we also de facto own it and will continue to do so. Let's anchor things at ./pkg/kv/kvserver/liveness for now.

Proto file movement is always safe. What's not generally safe is changing the package of proto files. This is unsafe in two cases:

  1. services were defined: that changes the name of the service, creating a migration concern
  2. any of the defs in the affected files are used with protobuf.Any, in which case again the package name is in general needed to work with the field.

Neither are an issue here, so I think it is safe to adjust the package name (on top of the location).

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm: mod my previous comment and thanks 🙇 for making this happen!

Reviewed 72 of 72 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)

@irfansharif irfansharif force-pushed the 201102.pkg-nodeliveness branch 2 times, most recently from 12b1f27 to 08ecd5a Compare November 3, 2020 13:51
Copy link
Contributor Author

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

TFTR!

Let's anchor things at ./pkg/kv/kvserver/liveness for now.

Done. I had it there originally, moving it out only to make the proto import paths a bit pithier, but not a good reason. Will bors on green. +cc @lunevalex, for the multiTestContext related details here.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @tbg)

NodeLiveness is a relatively isolated component and makes for a good
"should be a standalone package" candidate. We tease it out here, mostly
through mechanical code movements, in the hope that it'll force us to
clarify API boundaries going forward. Some intended changes to these
APIs, like introducing a way to read liveness records directly from KV,
will be added to this new package in other PRs. (These APIs were
originally proposed in cockroachdb#56107, and will be pulled out into stand alone
PRs after this one.)

While here, we also move out the liveness proto definition into this new
package. The "changes" made in this PR are the following:

  pkg/kv/kvserver/{node liveness files}               => pkg/kv/kvserver/liveness
  pkg/kv/kvserver/liveness/{node liveness protos}     => pkg/kv/kvserver/liveness/livenesspb
  pkg/kv/kvserver/liveness/node_liveness_test.go      => pkg/kv/kvserver/liveness/client_test.go
  pkg/kv/kvserver/liveness/node_liveness_unit_test.go => pkg/kv/kvserver/liveness/liveness_test.go

Separately, we've had to initialize server factory in TestMain as the
package internal tests depend on it. This was previously done within
pkg/kv/kvserver/main_test.go.

We've also renamed a few exported symbols to avoid stuttering. This was
needed to make our linters happy.

---

I've left the existing node liveness tests that make use of
multiTestContext in the kvserver package, seeing as how we're slowly
upgrading them to use TestCluster instead (cockroachdb#8299). The expectation is
that as we migrate each test, they'll move into this new package.

Release note: None
@irfansharif
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Nov 3, 2020

Build succeeded:

@craig craig bot merged commit e533f19 into cockroachdb:master Nov 3, 2020
@irfansharif irfansharif deleted the 201102.pkg-nodeliveness branch November 3, 2020 18:03
craig bot pushed a commit that referenced this pull request Nov 3, 2020
56130: opt: updated normalization rules for folding is expressions with null predicate r=mgartner a=jayshrivastava

opt: updated normalization rules for folding is expressions with null predicate

Previously, for statements such as `SELECT (foo,bar) IS DISTINCT FROM NULL FROM a_table`,
the expression `(foo,bar) IS DISTINCT FROM NULL` would not be normalized to `true`.
Similarly, if `IS NOT DISTINCT FROM NULL` were used, then the expression would not be
normalized to `false`. The previous statement would only normalize if the tuple/array in
the statement contained only constants. Given the updates in this commit, normalization
will be applied when any arrays or tuples are provided in this situation.

Release note: None

56217: sql: clean up uses of Statement r=RaduBerinde a=RaduBerinde

This change cleans up the use of `sql.Statement` and reduces some
allocations. Specifically:

 - we create a  `Statement` lower in the stack (in `execStmtInOpenState`),
   and pass only what we need in the higher layers;

 - we change various functions to take a `tree.Statement` rather than
   an entire `Statement` when possible;

 - we move down the `withStatement` context allocation, so that it is
   avoided in the implicit transaction state transition;

 - we store a copy rather than a pointer to the Statement in the
   planner;

 - we avoid directly using `stmt` fields from `func()` declarations
   that escape;

 - we populate `Statement.AnonymizedStr` upfront. The anonymized
   string is always needed (to update statement stats).

```
name                               old time/op    new time/op    delta
EndToEnd/kv-read/EndToEnd             153µs ± 1%     154µs ± 2%    ~     (p=0.486 n=4+4)
EndToEnd/kv-read-no-prep/EndToEnd     216µs ± 1%     217µs ± 1%    ~     (p=0.886 n=4+4)
EndToEnd/kv-read-const/EndToEnd       111µs ± 1%     113µs ± 1%  +1.01%  (p=0.029 n=4+4)

name                               old alloc/op   new alloc/op   delta
EndToEnd/kv-read/EndToEnd            25.8kB ± 1%    25.5kB ± 1%    ~     (p=0.114 n=4+4)
EndToEnd/kv-read-no-prep/EndToEnd    32.2kB ± 1%    31.9kB ± 1%    ~     (p=0.686 n=4+4)
EndToEnd/kv-read-const/EndToEnd      21.0kB ± 1%    20.7kB ± 2%    ~     (p=0.200 n=4+4)

name                               old allocs/op  new allocs/op  delta
EndToEnd/kv-read/EndToEnd               252 ± 1%       250 ± 0%  -0.99%  (p=0.029 n=4+4)
EndToEnd/kv-read-no-prep/EndToEnd       332 ± 0%       330 ± 1%    ~     (p=0.229 n=4+4)
EndToEnd/kv-read-const/EndToEnd         214 ± 0%       212 ± 0%  -0.93%  (p=0.029 n=4+4)
```

Release note: None

56243: liveness: introduce GetLivenessesFromKV r=irfansharif a=irfansharif

Now that we always create a liveness record on start up (#53805), we can simply
fetch all records from KV when wanting an up-to-date view of all nodes that
have ever been a part of the cluster. We add a helper to do as much, which
we'll rely on when introducing long running migrations (#56107).

It's a bit unfortunate that we're further adding on to the liveness API without
changing the underlying look-aside cache structure, but the up-to-date records
from KV directly is the world we're hoping to start moving towards over time.
The TODO added in [1] outlines what the future holds.

We'll also expose the GetLivenessesFromKV API we introduced earlier to pkg/sql.
We'll rely on it when needing to plumb in the liveness instance into the
migration manager process (prototyped in #56107)

It should be noted that this will be a relatively meatier form of a dependency
on node liveness from pkg/sql than we have currently. Today the only uses are
in DistSQL planning and in jobs[2]. As it relates to our multi-tenancy work,
the real use of this API will happen only on the system tenant. System tenants
alone have the privilege to set cluster settings (or at least the version
setting specifically), which is what the migration manager will be wired into.

[1]: d631239
[2]: #48795

Release note: None

---

First commit is from #56221, and can be ignored here.


Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
craig bot pushed a commit that referenced this pull request Nov 6, 2020
56382: liveness: clean up BUILD gunk r=irfansharif a=irfansharif

We had this leftover gunk from when we originally named this package
`nodeliveness` (#56221). Seems like bazel regeneration did not strip it 
all out when we moved things around.

Release note: None

Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
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.

3 participants