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

deps: bump go.etcd.io/etcd #38913

Merged
merged 3 commits into from
Jul 18, 2019
Merged

deps: bump go.etcd.io/etcd #38913

merged 3 commits into from
Jul 18, 2019

Conversation

danhhz
Copy link
Contributor

@danhhz danhhz commented Jul 16, 2019

To pick up etcd-io/etcd#10864.

Release note: None

@danhhz danhhz requested review from tbg and a team July 16, 2019 21:04
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@danhhz danhhz requested a review from a team July 16, 2019 22:21
Copy link
Contributor Author

@danhhz danhhz left a comment

Choose a reason for hiding this comment

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

Updated a bunch of stuff to the new naming, but I'm still seeing a couple tests fail with unexpected snapshots

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/split_delay_helper_test.go, line 111 at r2 (raw file):

							RecentActive: true,
							ProbeSent:    true, // Unifies string output below.
							Inflights:    &tracker.Inflights{},

This was panic'ing in (tracker.Progress).String if Inflights was nil. Do we care?

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.

I'm going to take a look at this branch. The upstream changes were invasive, I wouldn't be 100% surprised if there were some fallout.

Reviewed 2 of 2 files at r1, 14 of 14 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)


pkg/storage/split_delay_helper_test.go, line 111 at r2 (raw file):

Previously, danhhz (Daniel Harrison) wrote…

This was panic'ing in (tracker.Progress).String if Inflights was nil. Do we care?

That's OK, Inflights is never nil inside of Raft. BTW there is tracker.NewInflights.

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.

Reviewed 18 of 18 files at r3, 2 of 2 files at r4, 3 of 3 files at r5, 14 of 14 files at r6.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)

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.

Ok, this wasn't hard to figure out. First of all I was wrong about the Inflights field, we nilled that intentionally sometimes, which was a bad idea and I'm fixing it in etcd-io/etcd#10903; updated the vendor PR to pick up this fix (when it merges I have to amend the commit to vendor from the upstream repo, but I want to see if tests turn green).

The denied snapshot thing was actually discarded snapshots because I put more safety checks into raft and of course preemptive snapshots failed them. We need to lie a little more when creating the temporary raft group that applies the snap. See the corresponding commit.

Have some hope that this will now turn green, we'll see.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @danhhz)

@tbg
Copy link
Member

tbg commented Jul 17, 2019

Yep, it's green. So we just have to get etcd-io/etcd#10903 merged and update the vendored ref.

tbg and others added 3 commits July 18, 2019 09:54
It was printing a non-stringer as %s.

Release note: None
As of recenly, Raft [checks] that a peer receiving a snapshot is
actually contained in the configuration of a snapshot.  Preemptive
snapshots were failing that check because we applied them via a
temporary Raft group that had ID MaxUint64.

Preemptive snapshots are already a lie, so until we get rid of them
completely we'll have to lie a little more and use an ID that is
actually in the config. Any ID from the config will do; we take that of
the sending replica. Additionally, we're a little more careful than
previously to not leak anything about this temporary Raft group to the
outside world - previously we allowed it to send messages. This was
always a bad idea, but under a real ID this would be outright byzantine.

[checks]: https://github.com/etcd-io/etcd/blob/aa158f36b9832038a38f611d075264acb3874c8c/raft/raft.go#L1357-L1362

Release note: None
To pick up etcd-io/etcd#10864.

This commit also resolves the following, which have happened since the
last version of etcd/raft:
- The raft.ProgressState enum and its variants now live in raft/tracker.
- The Paused field is no longer present on raft.Progress.
- RawNode.Status no longer returns a pointer.
- Some of the fields on raft.Status are now on an embedded
  raft.BasicStatus struct.

Release note: None
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:

Reviewed 19 of 19 files at r7, 2 of 2 files at r8, 17 of 17 files at r9.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @danhhz)

@danhhz
Copy link
Contributor Author

danhhz commented Jul 18, 2019

TFTR!

bors r=tbg

craig bot pushed a commit that referenced this pull request Jul 18, 2019
38904: parser: Add user defined column families to CTAS. r=adityamaru27 a=adityamaru27

This change introduces grammar to support user defined
column families in a CREATE TABLE ... AS query.

38913: deps: bump go.etcd.io/etcd r=tbg a=danhhz

To pick up etcd-io/etcd#10864.

Release note: None

38972: exec: fix calculation of selectivity stat when num batches is zero r=yuzefovich a=yuzefovich

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Daniel Harrison <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jul 18, 2019

Build succeeded

@craig craig bot merged commit 3315afa into cockroachdb:master Jul 18, 2019
@danhhz danhhz deleted the raft branch September 3, 2019 19:02
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