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

Node start #33630

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Node start #33630

wants to merge 12 commits into from

Conversation

andreimatei
Copy link
Contributor

No description provided.

@andreimatei andreimatei requested a review from a team January 10, 2019 18:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

…er id

The Gossip already is handles the cluster id key specially, so I've
added more convenience methods for dealing with it. Besides convenience,
they also make it clear that this info doesn't have a TTL.

Release note: None
Before this patch, the initServer was complicated - it had some sort of
a semaphore exposed with an unclear API. It was also performing
bootstrapping of the cluster, duplicating code with the Server which
performs bootstrap in cases where an initServer is not used.
No more. This patch reduces the scope of the initServer to be a
glorified channel signaled when bootstrap is requested. Bootstrapping is
left to the calling Server, and so code is unified.

This patch is part of a somewhat chaotic assault on the cluster
bootstrap and generally node startup code, which I find inscrutable.
Other patches flying by stem from this unravelling sweater. One of the
goals is to rework bootstrapping such that the nodeIDContainer hack goes
away.

Release note: None
... making 2.2 nodes incompatible with 2.0 nodes.
This lets me assume 2.1 nodes (and do the next commit). The instructions
for when to bump this say to bump this when we introduce 2_2, but I
think that's off (or, rather, we should already have introduced 2.2).
Also see cockroachdb#33578.

Release note: None
This migration is complicated. The patch gets rid of the version guard,
but not otherwise the migration code. Before this patch, the migration
for a range was triggered when a range was not migrated yet and
IsMinSupported(VersionRangeAppliedStateKey). The latter part is always
true: since a previous commit set BinaryMinimumSupportedVersion=2_1,
VersionRangeAppliedStateKey was guaranteed.
After this patch, the migration is triggered when a range was not yet
migrated.
It would seem that we have to live with this migration for an undefined
period of time longer: some ranges might have never been migrated if
they originated in an, say, 1.1 cluster that went through a series of
upgrades but the range was never touched.

One thing that was considered was having the below-Raft command
application always perform the migration if the range wasn't previously
migrated, regardless of what the proposal specifies. This doesn't seem
feasible since we need to guarantee that different replicas apply the
command exactly the same, and so we can't have one ignore the proposal
spec and another one not ignore it. We'll be able to unconditionally
perform the migration once we decree that there can no longer be
proposals "in flight" coming from ancient nodes (think unapplied Raft
lags).

Release note: None
That method was quite pitiful, a shell of some 2014 glory. Its comment
was wrong, one argument was unused, one return value was always nil.
The code is better without its 3 lines too.

Release note: None
Release note: None
There was some really nasty code that's luckily now unused (I think
likely it became unused in cockroachdb#33150).
More rationalizing of that method coming next.

Release note: None
That method is used for writing initial data to an engine. Before this
patch it was using a Store, but for no good reason. This meant that we
had to create a Store only to stop it immediately and re-create it later
with the same engine.
This patch makes the method free-standing and it now takes an engine.

Release note: None
@tbg tbg added the X-noremind Bots won't notify about PRs with X-noremind label Jun 19, 2019
@bobvawter
Copy link
Member

bobvawter commented Mar 10, 2020

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-noremind Bots won't notify about PRs with X-noremind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants