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

*: refactor consistentindex #11699

Merged
merged 3 commits into from
Mar 26, 2020
Merged

Conversation

tangcong
Copy link
Contributor

consistentIndex is a global concept. It has nothing to do with mvcc. this pr separate it from mvcc/store and encapsulate it in a separate package. When mvcc, lessor, auth and other modules need to prevent repeated execution of commands, call consistentIndex package Save methods to store.

@tangcong tangcong force-pushed the refactor-consistentindex branch 3 times, most recently from 305ceb9 to 2da44a9 Compare March 16, 2020 03:23
@codecov-io
Copy link

codecov-io commented Mar 16, 2020

Codecov Report

Merging #11699 into master will increase coverage by 0.84%.
The diff coverage is 97.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11699      +/-   ##
==========================================
+ Coverage   66.40%   67.24%   +0.84%     
==========================================
  Files         402      403       +1     
  Lines       36667    37441     +774     
==========================================
+ Hits        24349    25179     +830     
+ Misses      10829    10762      -67     
- Partials     1489     1500      +11     
Impacted Files Coverage Δ
clientv3/snapshot/util.go 0.00% <ø> (-20.00%) ⬇️
mvcc/kvstore.go 86.90% <88.88%> (-0.83%) ⬇️
etcdserver/cindex/cindex.go 96.87% <96.87%> (ø)
auth/store.go 76.69% <100.00%> (+13.20%) ⬆️
clientv3/snapshot/v3_snapshot.go 59.76% <100.00%> (+0.32%) ⬆️
etcdserver/backend.go 56.25% <100.00%> (ø)
etcdserver/server.go 76.05% <100.00%> (+0.26%) ⬆️
mvcc/watchable_store.go 84.94% <100.00%> (+0.60%) ⬆️
pkg/testutil/recorder.go 77.77% <0.00%> (-3.71%) ⬇️
proxy/grpcproxy/watch.go 89.94% <0.00%> (-2.96%) ⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d1982e...631c23d. Read the comment docs.

@tangcong tangcong changed the title *": refactor consistentindex *: refactor consistentindex Mar 16, 2020
Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

Thanks for helping refactoring the code! Looks good in general!

Procfile Outdated Show resolved Hide resolved
auth/store.go Outdated Show resolved Hide resolved
etcdserver/cindex/cindex.go Outdated Show resolved Hide resolved
etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/server.go Outdated Show resolved Hide resolved
etcdserver/server_test.go Outdated Show resolved Hide resolved
auth/store.go Outdated Show resolved Hide resolved
return ConsistentIndex{bytesBuf8: make([]byte, 8)}
}

func (cindex *ConsistentIndex) LoadConsistentIndex(tx backend.BatchTx) uint64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason for having a separate function to load index? In the old implementation this is part of ConsistentIndex().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. if we do not have LoadConsistentIndex method, we have to pass batch.Tx parameter to ConsistentIndex Method(consistent index is zero after starting etcd server).

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried to pass batch Tx when creating new consistent index? Do you feel it provides better encapsulation?

Copy link
Contributor Author

@tangcong tangcong Mar 18, 2020

Choose a reason for hiding this comment

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

i am trying to pass batch Tx when creating new consistent index. there are so many test case parameters need to be changed and it seems batch Tx be closed sometimes. I will deep dive it tomorrow.
https://travis-ci.com/github/etcd-io/etcd/jobs/299521485

anic: assertion failed: tx closed
goroutine 895 [running]:
go.etcd.io/etcd/vendor/go.etcd.io/bbolt._assert(...)
 /Users/simotang/go/src/go.etcd.io/etcd/vendor/go.etcd.io/bbolt/db.go:1172
go.etcd.io/etcd/vendor/go.etcd.io/bbolt.(*Cursor).seek(0xc006f4ede0, 0x2556520, 0x4, 0x4, 0x1bfeea9, 0x1d, 0x11, 0x2, 0x0, 0x0, ...)
 /Users/simotang/go/src/go.etcd.io/etcd/vendor/go.etcd.io/bbolt/cursor.go:155 +0x182
go.etcd.io/etcd/vendor/go.etcd.io/bbolt.(*Bucket).Bucket(0xc0001c8718, 0x2556520, 0x4, 0x4, 0x0)
 /Users/simotang/go/src/go.etcd.io/etcd/vendor/go.etcd.io/bbolt/bucket.go:105 +0xd5

Copy link
Contributor Author

@tangcong tangcong Mar 19, 2020

Choose a reason for hiding this comment

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

i see. oldbe will be closed when etcdserver executes applySnapshot command. no other commands will result in backend being closed. i have tried to add a SetBatchTx method to solve this issue. it seems good. how do you think so ?

	lg.Info("restored mvcc store")

	// Closing old backend might block until all the txns
	// on the backend are finished.
	// We do not want to wait on closing the old backend.
	s.bemu.Lock()
	oldbe := s.be
	go func() {
		lg.Info("closing old backend file")
		defer func() {
			lg.Info("closed old backend file")
		}()
		if err := oldbe.Close(); err != nil {
			lg.Panic("failed to close old backend", zap.Error(err))
		}
	}()

	s.be = newbe
	s.bemu.Unlock()

Copy link
Contributor

@jingyih jingyih Mar 19, 2020

Choose a reason for hiding this comment

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

In the new implementation, consistentIndex and the store who uses it should always refer to the same backend. The store might close backend and open a new backend. Whenever this happens, it needs to update the backend used by consistentIndex. Is my understand correct?

If this is the case, I don't feel strongly which implementation has better encapsulation. Maybe keep the old way? (i.e., the caller of Load and Save methods needs to pass in the batchTx of the backend it is currently using)

Copy link
Contributor Author

@tangcong tangcong Mar 19, 2020

Choose a reason for hiding this comment

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

yes,we have to update the backend used by consistentIndex when old backend be closed. however,no other commands will close backend except applySnapshot. i think it is safe. if we keep the old way, we also have to call LoadConsistentIndex when backend being closed(restore consistent index from snapshot). meanwhile, we must firstly call LoadConsistentIndex when creating a NewConsistentIndex every time in old version. By comparison, the new version is more friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we call SetBatchTx in store.Restore function, it seems very good to solve this issue. there are only two function(NewStore/Restore) will update backend.

--- a/mvcc/kvstore.go
+++ b/mvcc/kvstore.go
@@ -333,6 +333,8 @@ func (s *store) Restore(b backend.Backend) error {
        s.compactMainRev = -1
        s.fifoSched = schedule.NewFIFOScheduler()
        s.stopc = make(chan struct{})
+       s.ci.SetConsistentIndex(0)
+       s.ci.SetBatchTx(b.BatchTx())

Copy link
Contributor

Choose a reason for hiding this comment

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

OK let's go with the new implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. i have updated pr and travis ci passes all test cases.

clientv3/snapshot/util.go Outdated Show resolved Hide resolved
@tangcong tangcong force-pushed the refactor-consistentindex branch 4 times, most recently from a5d5ad3 to 9cc877d Compare March 18, 2020 08:53
Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

Thank you!

etcdserver/cindex/cindex.go Outdated Show resolved Hide resolved
etcdserver/backend.go Outdated Show resolved Hide resolved
clientv3/snapshot/v3_snapshot.go Show resolved Hide resolved
return ConsistentIndex{bytesBuf8: make([]byte, 8)}
}

func (cindex *ConsistentIndex) LoadConsistentIndex(tx backend.BatchTx) uint64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried to pass batch Tx when creating new consistent index? Do you feel it provides better encapsulation?

@tangcong tangcong force-pushed the refactor-consistentindex branch 7 times, most recently from 7c2a22c to 30cfc36 Compare March 19, 2020 15:02
s.b = b
s.kvindex = newTreeIndex(s.lg)
s.currentRev = 1
s.compactMainRev = -1
s.fifoSched = schedule.NewFIFOScheduler()
s.stopc = make(chan struct{})
s.ci.SetBatchTx(b.BatchTx())
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple questions:

  1. I believe we also need this for auth store (both newstore and recover)?
  2. backend batchTx is recreated periodically (every 100ms or so), so we should pass backend instead of backend batchTx. Could you double check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. auth store and mvcc store share the same consistent indexer, so it is not necessary for auth store.
  2. batchTx.tx is recreated periodically, but we are passing batchTx to consistentIndexer, not batchTx.tx. There is only one function to reset batchTx (newBackend), so it looks ok.
    image

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Thanks!

etcdserver/cindex/cindex.go Outdated Show resolved Hide resolved
@tangcong
Copy link
Contributor Author

@jingyih could you have other advice?

// so ConsistentWatchableKV could get the consistent index from it.

type consistentIndex struct {
tx backend.BatchTx
Copy link
Contributor

Choose a reason for hiding this comment

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

One more question. Do we need to move tx below consistentIndex to keep consistentIndex 64-bit aligned?

Copy link
Contributor Author

@tangcong tangcong Mar 25, 2020

Choose a reason for hiding this comment

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

sizeof(interface) is 8 bytes in 32 bit,16 bytes in 64 bit. so we do not need to move tx below consistentIndex.
https://play.golang.org/p/9Bj6zdtyehz

Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@xiang90
Copy link
Contributor

xiang90 commented Mar 25, 2020

/cc @gyuho can you take a final look? thanks!

@gyuho
Copy link
Contributor

gyuho commented Mar 26, 2020

lgtm thx!

@gyuho gyuho merged commit 3ac7a11 into etcd-io:master Mar 26, 2020
tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020
tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020
tangcong added a commit to tangcong/etcd that referenced this pull request Apr 5, 2020
jingyih added a commit that referenced this pull request Apr 10, 2020
@tangcong tangcong deleted the refactor-consistentindex branch February 26, 2021 18:59
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.

5 participants