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

storage: update import path for Raft #29574

Merged
merged 1 commit into from
Sep 5, 2018
Merged

Conversation

tbg
Copy link
Member

@tbg tbg commented Sep 5, 2018

upstream moved from github.com/etcd to go.etcd.io/etcd. We're going
to have to pick up an update soon to fix #28918, so it's easier to
switch now.

We're not picking up any new commits except for the renames.

Release note: None

@tbg tbg requested a review from a team September 5, 2018 07:46
@tbg tbg requested a review from a team as a code owner September 5, 2018 07:46
@tbg tbg requested a review from a team September 5, 2018 07:46
@cockroach-teamcity
Copy link
Member

This change is Reviewable

upstream moved from github.com/etcd to go.etcd.io/etcd. We're going
to have to pick up an update soon to fix cockroachdb#28918, so it's easier to
switch now.

We're not picking up any new commits except for the renames.

Release note: None
Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: for the renames. Small question about the different SHA and whether a version bump now is with the risk vs your other PR which works around the problem solely in cockroach code.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


Gopkg.lock, line 1317 at r1 (raw file):

  ]
  pruneopts = "UT"
  revision = "1df1ddff4361ed7f2c0f33571923511889a115ce"

Should I worry that this is a different sha? Are we getting an etcd/raft bump at the same time?

Copy link
Member Author

@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.

TFTR. There should be zero risk in this PR since we're not picking up any new code.
I'd also like to pick up the upstream fix when it's merged, though perhaps early in the 2.2 cycle.
In any case, the next time we bump Raft we'll have a good reason to do so, so it will be nice to not shave this yak then.

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


Gopkg.lock, line 1317 at r1 (raw file):
No, see

We're not picking up any new commits except for the renames.

We are picking up the renames, so the SHA is different. See https://github.com/etcd-io/etcd/commits/master/raft

Copy link
Member Author

@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.

bors r=petermattis

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

@craig
Copy link
Contributor

craig bot commented Sep 5, 2018

👎 Rejected by code reviews

Copy link
Member Author

@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.

Dismissed @petermattis from a discussion.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

Copy link
Member Author

@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.

bors r=petermattis

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

@craig
Copy link
Contributor

craig bot commented Sep 5, 2018

👎 Rejected by code reviews

@tbg
Copy link
Member Author

tbg commented Sep 5, 2018

bors r=petermattis

craig bot pushed a commit that referenced this pull request Sep 5, 2018
29574: storage: update import path for Raft r=petermattis a=tschottdorf

upstream moved from github.com/etcd to go.etcd.io/etcd. We're going
to have to pick up an update soon to fix #28918, so it's easier to
switch now.

We're not picking up any new commits except for the renames.

Release note: None

29585: gossip: avoid allocation of UnresolvedAddr in getNodeIDAddressLocked r=nvanbenschoten a=nvanbenschoten

`getNodeIDAddressLocked` is called from `Dialer.ConnHealth` and
`Dialer.DialInternalClient`. It was responsible for **1.71%** of all
allocations (`alloc_objects`) on a 3-node long-running cluster that
was running TPC-C 1K.

Pointing into `nd.LocalityAddress` is safe because even if the `NodeDescriptor`
itself is replaced in `Gossip`, the struct is never internally mutated. This is
the same reason why taking the address of `nd.Address` was already safe.

Release note (performance improvement): Avoid allocation when
checking RPC connection health.

Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 5, 2018

Build succeeded

@craig craig bot merged commit 3afbd21 into cockroachdb:master Sep 5, 2018
packages = ["."]
pruneopts = "UT"
revision = "bb4de0191aa41b5507caa14b0650cdbddcd9280b"

Copy link
Contributor

Choose a reason for hiding this comment

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

Um. This should not have gotten deleted as far as I can tell. Looking into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks and apologies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not your fault at all! That frustration was directed at dep.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I could've seen something unexpected was happening. :-)

@tbg tbg deleted the fix/raft-vendor branch September 5, 2018 16:33
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.

storage: pick up upstream fix for Raft entries handling
4 participants