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

Update raft dep #32938

Merged
merged 1 commit into from
Dec 7, 2018
Merged

Update raft dep #32938

merged 1 commit into from
Dec 7, 2018

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Dec 7, 2018

go.etcd.io/etcd (024f3df -> 1900a8e)

Release note: None

I doin't know where I can find a web interface to show the commits. I pasted the log below.

commit 1900a8e26f2c3d46ed9ebdb4c876578efeb9bce4
Merge: 8a9a2a1a5 bfaae1ba4
Author: Xiang Li <[email protected]>
Date:   Thu Dec 6 10:58:22 2018 -0800

    Merge pull request #10308 from tbg/fix/progress-after-snap
    
    raft: enter ProgressStateReplica immediately after snapshot

commit bd332b318e5e805352f763989f8d1971a9f19955
Author: Tobias Schottdorf <[email protected]>
Date:   Thu Dec 6 19:02:47 2018 +0100

    raft: add (*RawNode).WithProgress
    
    Calls to Status can be frequent and currently incur three heap
    allocations, but often the caller has no intention to hold on to the
    returned status.
    
    Add StatusWithoutProgress and WithProgress to allow avoiding heap
    allocations altogether. StatusWithoutProgress does what's on the
    tin and additionally returns a value (instead of a pointer) to
    avoid the associated heap allocation. By not returning a Progress
    map, it avoids all other allocations that Status incurs.
    
    To still introspect the Progress map, add WithProgress, which
    uses a simple visitor pattern.
    
    Add benchmarks to verify that this is indeed allocation free.
    
    ```
    BenchmarkStatusProgress/members=1/Status-8                  5000000    353 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=1/Status-example-8          5000000    372 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=1/StatusWithoutProgress-8   100000000  17.6 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=1/WithProgress-8            30000000   48.6 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=1/WithProgress-example-8    30000000   42.9 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=3/Status-8                  5000000    395 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=3/Status-example-8          3000000    449 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=3/StatusWithoutProgress-8   100000000  18.7 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=3/WithProgress-8            20000000   78.1 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=3/WithProgress-example-8    20000000   70.7 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=5/Status-8                  3000000    470 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=5/Status-example-8          3000000    544 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=5/StatusWithoutProgress-8   100000000  19.7 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=5/WithProgress-8            20000000   105 ns/op          0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=5/WithProgress-example-8    20000000   94.0 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=100/Status-8                100000     11903 ns/op    22663 B/op   12 allocs/op
    BenchmarkStatusProgress/members=100/Status-example-8        100000     13330 ns/op    22669 B/op   12 allocs/op
    BenchmarkStatusProgress/members=100/StatusWithoutProgress-8 50000000   20.9 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=100/WithProgress-8          1000000    1731 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=100/WithProgress-example-8  1000000    1571 ns/op         0 B/op    0 allocs/op
    ```

commit bfaae1ba462c91aaf149a285b8d2369807044f71
Author: Tobias Schottdorf <[email protected]>
Date:   Thu Dec 6 11:09:46 2018 +0100

    raft: enter ProgressStateReplica immediately after snapshot
    
    When a follower requires a snapshot and the snapshot is sent at the
    committed (and last) index in an otherwise idle Raft group, the follower
    would previously remain in ProgressStateProbe even though it had been
    caught up completely.
    
    In a busy Raft group this wouldn't be an issue since the next round of
    MsgApp would update the state, but in an idle group there's nothing
    that rectifies the status (since there's nothing to append or update).
    
    The reason this matters is that the state is exposed through
    `RaftStatus()`. Concretely, in CockroachDB, we use the Raft status to
    make sharding decisions (since it's dangerous to make rapid changes
    to a fragile Raft group), and had to work around this problem[1].
    
    [1]: https://github.com/cockroachdb/cockroach/blob/91b11dae416f3d9b55fadd2a4b096e94d874176c/pkg/storage/split_delay_helper.go#L138-L141

commit 6c649de36e0b0df51ce356a0f353203b9580f6e1
Merge: f28945ba8 1569f4829
Author: Xiang Li <[email protected]>
Date:   Sat Nov 24 11:48:16 2018 +0800

    Merge pull request #10281 from tbg/print-hint-reject-app-resp
    
    raft: print RejectHint of zero on MsgAppResp

commit 5c209d66d2ab8a5de7a33a61a1f90c4a4caefd7c
Author: Tobias Schottdorf <[email protected]>
Date:   Fri Nov 23 17:57:36 2018 +0100

    raft: ensure leader is in ProgressStateReplicate
    
    The leader perpetually kept itself in ProgressStateProbe even though of
    course it has perfect knowledge of its log. This wasn't usually an issue
    because it also doesn't care about its own Progress, but it's better to
    keep this data correctly maintained, especially since this is part of
    raft.Status and thus becomes visible to applications using the Raft
    library.
    
    (Concretely, in CockroachDB we use the Progress to inform log
    truncations).

commit 1569f4829de52927593a1d99a049eec01ec88ff2
Author: Tobias Schottdorf <[email protected]>
Date:   Fri Nov 23 11:06:38 2018 +0100

    raft: print RejectHint of zero on MsgAppResp
    
    A zero RejectHint on MsgAppResp is still used, and so should be
    reflected in the message description.

go.etcd.io/etcd (024f3df -> 1900a8e)

Release note: None
@RaduBerinde RaduBerinde requested review from tbg and a team December 7, 2018 19:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@tbg
Copy link
Member

tbg commented Dec 7, 2018

commit 1900a8e
Merge: 8a9a2a1 bfaae1b
Author: Xiang Li [email protected]
Date: Thu Dec 6 10:58:22 2018 -0800

Merge pull request #10308 from tbg/fix/progress-after-snap

raft: enter ProgressStateReplica immediately after snapshot

commit bd332b3
Author: Tobias Schottdorf [email protected]
Date: Thu Dec 6 19:02:47 2018 +0100

raft: add (*RawNode).WithProgress

Calls to Status can be frequent and currently incur three heap
allocations, but often the caller has no intention to hold on to the
returned status.

Add StatusWithoutProgress and WithProgress to allow avoiding heap
allocations altogether. StatusWithoutProgress does what's on the
tin and additionally returns a value (instead of a pointer) to
avoid the associated heap allocation. By not returning a Progress
map, it avoids all other allocations that Status incurs.

To still introspect the Progress map, add WithProgress, which
uses a simple visitor pattern.

Add benchmarks to verify that this is indeed allocation free.

```
BenchmarkStatusProgress/members=1/Status-8                  5000000    353 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=1/Status-example-8          5000000    372 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=1/StatusWithoutProgress-8   100000000  17.6 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=1/WithProgress-8            30000000   48.6 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=1/WithProgress-example-8    30000000   42.9 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=3/Status-8                  5000000    395 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=3/Status-example-8          3000000    449 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=3/StatusWithoutProgress-8   100000000  18.7 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=3/WithProgress-8            20000000   78.1 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=3/WithProgress-example-8    20000000   70.7 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=5/Status-8                  3000000    470 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=5/Status-example-8          3000000    544 ns/op        784 B/op    3 allocs/op
BenchmarkStatusProgress/members=5/StatusWithoutProgress-8   100000000  19.7 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=5/WithProgress-8            20000000   105 ns/op          0 B/op    0 allocs/op
BenchmarkStatusProgress/members=5/WithProgress-example-8    20000000   94.0 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=100/Status-8                100000     11903 ns/op    22663 B/op   12 allocs/op
BenchmarkStatusProgress/members=100/Status-example-8        100000     13330 ns/op    22669 B/op   12 allocs/op
BenchmarkStatusProgress/members=100/StatusWithoutProgress-8 50000000   20.9 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=100/WithProgress-8          1000000    1731 ns/op         0 B/op    0 allocs/op
BenchmarkStatusProgress/members=100/WithProgress-example-8  1000000    1571 ns/op         0 B/op    0 allocs/op
```

commit bfaae1b
Author: Tobias Schottdorf [email protected]
Date: Thu Dec 6 11:09:46 2018 +0100

raft: enter ProgressStateReplica immediately after snapshot

When a follower requires a snapshot and the snapshot is sent at the
committed (and last) index in an otherwise idle Raft group, the follower
would previously remain in ProgressStateProbe even though it had been
caught up completely.

In a busy Raft group this wouldn't be an issue since the next round of
MsgApp would update the state, but in an idle group there's nothing
that rectifies the status (since there's nothing to append or update).

The reason this matters is that the state is exposed through
`RaftStatus()`. Concretely, in CockroachDB, we use the Raft status to
make sharding decisions (since it's dangerous to make rapid changes
to a fragile Raft group), and had to work around this problem[1].

[1]: https://github.com/cockroachdb/cockroach/blob/91b11dae416f3d9b55fadd2a4b096e94d874176c/pkg/storage/split_delay_helper.go#L138-L141

commit 6c649de
Merge: f28945b 1569f48
Author: Xiang Li [email protected]
Date: Sat Nov 24 11:48:16 2018 +0800

Merge pull request #10281 from tbg/print-hint-reject-app-resp

raft: print RejectHint of zero on MsgAppResp

commit 5c209d6
Author: Tobias Schottdorf [email protected]
Date: Fri Nov 23 17:57:36 2018 +0100

raft: ensure leader is in ProgressStateReplicate

The leader perpetually kept itself in ProgressStateProbe even though of
course it has perfect knowledge of its log. This wasn't usually an issue
because it also doesn't care about its own Progress, but it's better to
keep this data correctly maintained, especially since this is part of
raft.Status and thus becomes visible to applications using the Raft
library.

(Concretely, in CockroachDB we use the Progress to inform log
truncations).

commit 1569f48
Author: Tobias Schottdorf [email protected]
Date: Fri Nov 23 11:06:38 2018 +0100

raft: print RejectHint of zero on MsgAppResp

A zero RejectHint on MsgAppResp is still used, and so should be
reflected in the message description.

@tbg
Copy link
Member

tbg commented Dec 7, 2018

Fixed.

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.

PS, the repo is at https://github.com/etcd-io/etcd/tree/master/raft. The vanity import path is annoying.

@RaduBerinde
Copy link
Member Author

Good point, used git log 024f3dfc..1900a8e2 . and updated the description.

@RaduBerinde
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request Dec 7, 2018
32938: Update raft dep r=RaduBerinde a=RaduBerinde

go.etcd.io/etcd (024f3df -> 1900a8e)

Release note: None

I doin't know where I can find a web interface to show the commits. I pasted the log below.

```
commit 1900a8e
Merge: 8a9a2a1 bfaae1b
Author: Xiang Li <[email protected]>
Date:   Thu Dec 6 10:58:22 2018 -0800

    Merge pull request #10308 from tbg/fix/progress-after-snap
    
    raft: enter ProgressStateReplica immediately after snapshot

commit bd332b3
Author: Tobias Schottdorf <[email protected]>
Date:   Thu Dec 6 19:02:47 2018 +0100

    raft: add (*RawNode).WithProgress
    
    Calls to Status can be frequent and currently incur three heap
    allocations, but often the caller has no intention to hold on to the
    returned status.
    
    Add StatusWithoutProgress and WithProgress to allow avoiding heap
    allocations altogether. StatusWithoutProgress does what's on the
    tin and additionally returns a value (instead of a pointer) to
    avoid the associated heap allocation. By not returning a Progress
    map, it avoids all other allocations that Status incurs.
    
    To still introspect the Progress map, add WithProgress, which
    uses a simple visitor pattern.
    
    Add benchmarks to verify that this is indeed allocation free.
    
    ```
    BenchmarkStatusProgress/members=1/Status-8                  5000000    353 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=1/Status-example-8          5000000    372 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=1/StatusWithoutProgress-8   100000000  17.6 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=1/WithProgress-8            30000000   48.6 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=1/WithProgress-example-8    30000000   42.9 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=3/Status-8                  5000000    395 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=3/Status-example-8          3000000    449 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=3/StatusWithoutProgress-8   100000000  18.7 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=3/WithProgress-8            20000000   78.1 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=3/WithProgress-example-8    20000000   70.7 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=5/Status-8                  3000000    470 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=5/Status-example-8          3000000    544 ns/op        784 B/op    3 allocs/op
    BenchmarkStatusProgress/members=5/StatusWithoutProgress-8   100000000  19.7 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=5/WithProgress-8            20000000   105 ns/op          0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=5/WithProgress-example-8    20000000   94.0 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=100/Status-8                100000     11903 ns/op    22663 B/op   12 allocs/op
    BenchmarkStatusProgress/members=100/Status-example-8        100000     13330 ns/op    22669 B/op   12 allocs/op
    BenchmarkStatusProgress/members=100/StatusWithoutProgress-8 50000000   20.9 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=100/WithProgress-8          1000000    1731 ns/op         0 B/op    0 allocs/op
    BenchmarkStatusProgress/members=100/WithProgress-example-8  1000000    1571 ns/op         0 B/op    0 allocs/op
    ```

commit bfaae1b
Author: Tobias Schottdorf <[email protected]>
Date:   Thu Dec 6 11:09:46 2018 +0100

    raft: enter ProgressStateReplica immediately after snapshot
    
    When a follower requires a snapshot and the snapshot is sent at the
    committed (and last) index in an otherwise idle Raft group, the follower
    would previously remain in ProgressStateProbe even though it had been
    caught up completely.
    
    In a busy Raft group this wouldn't be an issue since the next round of
    MsgApp would update the state, but in an idle group there's nothing
    that rectifies the status (since there's nothing to append or update).
    
    The reason this matters is that the state is exposed through
    `RaftStatus()`. Concretely, in CockroachDB, we use the Raft status to
    make sharding decisions (since it's dangerous to make rapid changes
    to a fragile Raft group), and had to work around this problem[1].
    
    [1]: https://github.com/cockroachdb/cockroach/blob/91b11dae416f3d9b55fadd2a4b096e94d874176c/pkg/storage/split_delay_helper.go#L138-L141

commit 6c649de
Merge: f28945b 1569f48
Author: Xiang Li <[email protected]>
Date:   Sat Nov 24 11:48:16 2018 +0800

    Merge pull request #10281 from tbg/print-hint-reject-app-resp
    
    raft: print RejectHint of zero on MsgAppResp

commit 5c209d6
Author: Tobias Schottdorf <[email protected]>
Date:   Fri Nov 23 17:57:36 2018 +0100

    raft: ensure leader is in ProgressStateReplicate
    
    The leader perpetually kept itself in ProgressStateProbe even though of
    course it has perfect knowledge of its log. This wasn't usually an issue
    because it also doesn't care about its own Progress, but it's better to
    keep this data correctly maintained, especially since this is part of
    raft.Status and thus becomes visible to applications using the Raft
    library.
    
    (Concretely, in CockroachDB we use the Progress to inform log
    truncations).

commit 1569f48
Author: Tobias Schottdorf <[email protected]>
Date:   Fri Nov 23 11:06:38 2018 +0100

    raft: print RejectHint of zero on MsgAppResp
    
    A zero RejectHint on MsgAppResp is still used, and so should be
    reflected in the message description.
```

Co-authored-by: Radu Berinde <[email protected]>
@RaduBerinde
Copy link
Member Author

PS, the repo is at https://github.com/etcd-io/etcd/tree/master/raft. The vanity import path is annoying.

Is there something preventing us from importing it from github instead?

@craig
Copy link
Contributor

craig bot commented Dec 7, 2018

Build succeeded

@craig craig bot merged commit 947aad2 into cockroachdb:master Dec 7, 2018
@RaduBerinde RaduBerinde deleted the update-raft branch December 7, 2018 20:58
@RaduBerinde
Copy link
Member Author

Informs #30774.

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