sql: introduce and use OptionalLiveness #48795
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
tbg
commented
May 13, 2020
tbg
force-pushed
the
optionalliveness
branch
2 times, most recently
from
8873328 to
02cec22
Compare
ajwerner
reviewed
May 18, 2020
There was a problem hiding this comment.
Seems like there's a test that needs an update. Otherwise
cc @spaskob
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @tbg)
pkg/jobs/helpers.go, line 92 at r2 (raw file):
func (nl *FakeNodeLiveness) IsLive(roachpb.NodeID) (bool, error) { return false, errors.New("unsupported here")
nit: use an error message that's a tad more specific and greppable
pkg/jobs/registry.go, line 1151 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
It's fine that we hit this with
runningOnNode == true, right? It will hitcurrentlyRunningbelow and bail out, and also this is the same for single-tenant jobs.
Yes, that sounds right
pkg/jobs/registry.go, line 538 at r2 (raw file):
nl, ok := nlw.Optional(47892) if !ok { // At most one container is running on behalf of a SQL tenant, so it must be
I hope we get that coordination right. Can we add a TODO here with my name on it to add a different leasing mechanism to not have to rely on this assumption?
|
bors r=ajwerner TFTR! |
|
bors r- oopsed something |
Canceled |
|
bors r=ajwerner |
Build failed |
tbg
force-pushed
the
optionalliveness
branch
2 times, most recently
from
361051f to
5a35b3e
Compare
tbg
force-pushed
the
optionalliveness
branch
2 times, most recently
from
ab44285 to
2b8e24a
Compare
|
Fixed lint bors r=ajwerner |
Build failed |
|
Fixed lint again (staticcheck this time, not sure why that didn't pop up before) bors r=ajwerner |
Merge conflict (retrying...) |
1 similar comment
Merge conflict (retrying...) |
Merge conflict |
This makes it optional to provide a NodeLiveness instance. The only uses are in DistSQL planning and jobs. In the former, it was used only to check whether peers were live; we can just avoid that check since no remote DistSQL planning occurs in multi-tenancy Phase 2. For jobs, it was used to decide about job adoption and cancellation, but again in multi-tenancy there is at most one SQL server running (for that tenant) and so all jobs should be adopted unconditionally. Release note: None
|
bors r=ajwerner |
Build succeeded |
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Nov 3, 2020
Expose the GetLivenessesFromKV API we introduced earlier to pkg/sql. We'll eventually use this to power long running migrations (cockroachdb#56107), plumbing the liveness instance into the migration manager process. 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[1]. 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]: cockroachdb#48795 Release note: None
irfansharif
added a commit
to irfansharif/cockroach
that referenced
this pull request
Nov 3, 2020
Expose the GetLivenessesFromKV API we introduced earlier to pkg/sql. We'll eventually use this to power long running migrations (cockroachdb#56107), plumbing the liveness instance into the migration manager process. 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[1]. 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]: cockroachdb#48795 Release note: None
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes it optional to provide a NodeLiveness instance.
The only uses are in DistSQL planning and jobs. In the former,
it was used only to check whether peers were live; we can just
avoid that check since no remote DistSQL planning occurs in
multi-tenancy Phase 2. For jobs, it was used to decide about job
adoption and cancellation, but again in multi-tenancy there is
at most one SQL server running (for that tenant) and so all jobs should
be adopted unconditionally.
Release note: None