i wonder if we are being too restrictive by requiring this. as far as i can tell, it's actually okay for the schema to change bootstrap has started as long as the joining node is receiving forwarded writes + the new keyspace/columnfamily. this is already required for correctness in normal bootstraps (without schema changes), so i'm not sure we should be more strict here.

maybe the more "correct" check here is that other nodes see the joining node and are forwarding writes (presumably if the joining node is in their gossip, this is true). i think the current code already tries to do this via sleeping for 30 seconds to let gossip work and checking that gossip has seen the seeds

            Gossiper.instance.addLocalApplicationStates(states);
            setMode(Mode.JOINING, "sleeping " + RING_DELAY + " ms for pending range setup", true);
            Uninterruptibles.sleepUninterruptibly(RING_DELAY, TimeUnit.MILLISECONDS);
[...]

        if (!Gossiper.instance.seenAnySeed())
            throw new IllegalStateException("Unable to contact any seeds!");

but doesn't appear to guarantee that all writes are forwarded

I think you're right that this check is more restrictive than we need to be.

i think the order of this check is actually important. MigrationManager.isReadyForBootstrap() must be evaluated to true before initialLocalSchemaVersion.equals(Schema.instance.getVersion()) is evaluated to true. the way it's written now, it's possible for a partially applied schema to complete in between the two checks and not throw here

also what's the reason for putting these checks in Bootstrapper as opposed to StorageService#bootstrap?

The original idea was to put the second check in the callback for handling a PREPARE, but I decided that that was not necessary.

Moved the checks to SS

i don't see a reason not to just combine this with the previous check and have it be

            while (!MigrationManager.isReadyForBootstrap() || !isSchemaConsistent())
            {
                setMode(Mode.JOINING, "waiting for schema information to complete... local schema version is {Schema.instance.getVersion().toString()}", true);
                Uninterruptibles.sleepUninterruptibly(1, TimeUnit.SECONDS);
            }

this also means we can remove the above // first sleep the delay to make sure we see all our peers, right?

I've decided to switch the first loop to just a wait with no short circuit condition

i think we might as well just remove this given the below

I think having some delay here is still important because we know from experience that you should wait at least some amount of time here before proceeding. The intention of this change is to make the bootstrapping node wait

• at least 30s
• wait longer if it thinks the schema is consistent

A bad scenario would be if we get the non-empty schema by gossiping with a node in a network partition, think we have gossiped with everyone because we haven't waited long enough, and then pass the subsequent checks.

does that risk exist even if we wait for isSchemaConsistent? in that case, we should make our check more strict instead of just relying on a 30 second wait to hope it's safe

nit: return Gossiper.instance.getEndpointStates().stream().map(Entry::getValue).map(entry -> entry.getApplicationState(ApplicationState.SCHEMA).value).allMatch(Schema.instance.getVersion().toString()::equals);

👍

ah wait doesn't this check cause problems bc the endpoint states can have LEFT nodes?

I've made a change to exclude LEFT nodes, and also gracefully handle cases where ApplicationStates are null