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

(no)StoreV2 (Part 4): Backend hooks: precommit updates consistency_index #12855

Merged
merged 8 commits into from
May 8, 2021

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Apr 12, 2021

Implemented hooks mechanism in the backend, in particular pre-commit hook that:

  • guarantees code execution pre-every commit, even if the commit is triggered automatically on time or batch-size.
  • is potential place to add additional validations.

Integrated it with cindex, such that every transaction committed to backend is writing most recent consistent_index.

Makes sure that even automatically trigger commits of batch-transactions stays "really" consistent a.d. the most recent WAL log index applied. Prior to this PR membership and version edits were in particular not bumping up the 'cindex'.

Part of: #12913

@ptabor ptabor changed the title Backend hooks: precommit updates consistency_index (no)StoreV2 (Part 4): Backend hooks: precommit updates consistency_index Apr 12, 2021
Copy link
Contributor

@wpedrak wpedrak left a comment

Choose a reason for hiding this comment

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

LGTM. Few minor comments.

@ptabor ptabor requested review from gyuho and jingyih April 17, 2021 01:54
@ptabor ptabor added this to the etcd-v3.5 milestone Apr 17, 2021
@ptabor
Copy link
Contributor Author

ptabor commented Apr 21, 2021

@jingyih: I rebased the PR and its ready for review.

server/mvcc/backend/hooks_test.go Outdated Show resolved Hide resolved
@ptabor ptabor force-pushed the 20210409-backend-hooks branch 6 times, most recently from a9fd806 to d73e0c6 Compare April 29, 2021 16:20
ptabor added 2 commits May 4, 2021 15:38
Every transaction committed to backend is writing most recent consistent_index.
Makes sure that even automatically trigger commits of batch-transactions
stays "really" consistent a.d. the most recent WAL log index applied.
@ptabor ptabor force-pushed the 20210409-backend-hooks branch 2 times, most recently from 860b572 to 851584f Compare May 4, 2021 14:52
@ptabor
Copy link
Contributor Author

ptabor commented May 5, 2021

Not sure I fully understand this statement. It sounds to me that we improved / fixed something? Could you elaborate?

@jingyih
When I enabled it, I revealed several bugs (test flakes), that seems to be hidden by cindex lagging in DB:

I think that membership and version edits were in particular not bumping up the 'cindex' (as mvcc edits of backends were updating it, but not the direct edits to backend)

image

Added to PR description.


@gyuho please take a look at the proposed, more descriptive, names

@jingyih
Copy link
Contributor

jingyih commented May 5, 2021

@ptabor Thanks for the explanation. This makes sense to me. In the past all membership related raft entries (or more generally, all cluster related) are not tied to consistent index, as the consistent index was designed for kv store (as opposed to metadata bucket which stores cluster related info). With your changes, it makes the handling of raft entries of different types more consistent.

LGTM

@ptabor ptabor merged commit aeb9b5f into etcd-io:master May 8, 2021
@ptabor ptabor deleted the 20210409-backend-hooks branch May 8, 2021 07:34
@@ -104,7 +104,6 @@ func (tw *storeTxnWrite) Put(key, value []byte, lease lease.LeaseID) int64 {
func (tw *storeTxnWrite) End() {
// only update index if the txn modifies the mvcc state.
if len(tw.changes) != 0 {
tw.s.saveIndex(tw.tx)
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this breaks the atomicity of consistent index and writing transactions. @ptabor @ahrtr @serathius #13844

ahrtr added a commit to ahrtr/etcd that referenced this pull request Apr 1, 2022
serathius added a commit to serathius/etcd that referenced this pull request Apr 7, 2022
…rease

Reason to store CI and term in backend was to make db fully independent
snapshot, it was never meant to interfere with apply logic. Skip of CI
was introduced for v2->v3 migration where we wanted to prevent it from
decreasing when replaying wal in
etcd-io#5391. By mistake it was added to
apply flow during refactor in
etcd-io#12855 (comment).

Consistency index and term should only be negotiated and used by raft to make
decisions. Their values should only driven by raft state machine and
backend should only be responsible for storing them.
serathius added a commit to serathius/etcd that referenced this pull request Apr 7, 2022
…rease

Reason to store CI and term in backend was to make db fully independent
snapshot, it was never meant to interfere with apply logic. Skip of CI
was introduced for v2->v3 migration where we wanted to prevent it from
decreasing when replaying wal in
etcd-io#5391. By mistake it was added to
apply flow during refactor in
etcd-io#12855 (comment).

Consistency index and term should only be negotiated and used by raft to make
decisions. Their values should only driven by raft state machine and
backend should only be responsible for storing them.
serathius added a commit to serathius/etcd that referenced this pull request Apr 7, 2022
…rease

Reason to store CI and term in backend was to make db fully independent
snapshot, it was never meant to interfere with apply logic. Skip of CI
was introduced for v2->v3 migration where we wanted to prevent it from
decreasing when replaying wal in
etcd-io#5391. By mistake it was added to
apply flow during refactor in
etcd-io#12855 (comment).

Consistency index and term should only be negotiated and used by raft to make
decisions. Their values should only driven by raft state machine and
backend should only be responsible for storing them.
serathius added a commit to serathius/etcd that referenced this pull request Apr 7, 2022
…rease

Reason to store CI and term in backend was to make db fully independent
snapshot, it was never meant to interfere with apply logic. Skip of CI
was introduced for v2->v3 migration where we wanted to prevent it from
decreasing when replaying wal in
etcd-io#5391. By mistake it was added to
apply flow during refactor in
etcd-io#12855 (comment).

Consistency index and term should only be negotiated and used by raft to make
decisions. Their values should only driven by raft state machine and
backend should only be responsible for storing them.
tjungblu pushed a commit to tjungblu/etcd that referenced this pull request Sep 7, 2022
…rease

Reason to store CI and term in backend was to make db fully independent
snapshot, it was never meant to interfere with apply logic. Skip of CI
was introduced for v2->v3 migration where we wanted to prevent it from
decreasing when replaying wal in
etcd-io#5391. By mistake it was added to
apply flow during refactor in
etcd-io#12855 (comment).

Consistency index and term should only be negotiated and used by raft to make
decisions. Their values should only driven by raft state machine and
backend should only be responsible for storing them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants