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

WIP: integration: add v2v3 emulation #9634

Closed
wants to merge 1 commit into from

Conversation

yudai
Copy link
Contributor

@yudai yudai commented Apr 26, 2018

from #9626.

Run existing integration tests for v2 api (TestV2) on v2v3 emulation to improve the compatibility between them.

@yudai
Copy link
Contributor Author

yudai commented Apr 26, 2018

Currently tests are broken due to v2v3 emulation doesn't work properly for member list.

@yudai
Copy link
Contributor Author

yudai commented Apr 26, 2018

Are member API supposed to be supported by v2v3 emulation? Or it just supports basic KV operations...?

@heyitsanthony
Copy link
Contributor

@yudai the kv operations are the most tested; it still doesn't cover all membership operations. I left stubs for membership calls (see v2v3/cluster.go) with the intent that it would make v3 calls, convert to v2 structures, then let v2http handle the http response.

@yudai
Copy link
Contributor Author

yudai commented Apr 26, 2018

@heyitsanthony thanks for the pointer. I found that the methods of the api.Cluster interface don't have error in their return values, since the native v2 API uses internal values. However, if we use clientv3, it can cause errors. I'm wondering if we can change the interface (can breaking backward compatibility) to add error to return values.

TestV2Set is working now anyway. To make TestV2CreateUpdate, we need TTL support, which currently explicitly marked as errUnsupported for some reason. Is this just a stub?

@yudai
Copy link
Contributor Author

yudai commented Apr 26, 2018

Another potential issue I found is that stms used in store.go can be broken.

		for p := path.Dir(nodePath); !isRoot(p); p = path.Dir(p) {
			pp := s.mkPath(p)
			if stm.Rev(pp) > 0 {
				ecode = v2error.EcodeNotDir
				return nil
			}
			if stm.Rev(pp+"/") == 0 {
				dirs = append(dirs, pp+"/")
			}
		}
		for _, d := range dirs {
			stm.Put(d, "")
		}

If I understand correctly, in this code (from store.go), stm.Rev() is executed by a separate Tx from the main Tx for Put() operations. Therefore if there are other operations happen between the stm.Rev and the commit of the main Tx, it can cause inconsistency.
This these comp operations should be in a single Tx (STM doesn't have Comp() though)?

@heyitsanthony
Copy link
Contributor

heyitsanthony commented Apr 27, 2018

@yudai

wondering if we can change the interface (can breaking backward compatibility) to add error to return values.

I don't think adding error returns will break much. A lot of those old interfaces should be internal. Worst case, there can be some adapter to get the old behavior back.

if there are other operations happen between the stm.Rev and the commit of the main Tx, it can cause inconsistency.

For every key the txn is writing, the applier calls stm.Rev, which should insert the key into the readset (https://github.com/coreos/etcd/blob/master/clientv3/concurrency/stm.go#L291). If there's a change to any of those keys before the txn commits, STM will have a read conflict, throw away the puts, and retry. Is that not happening or is there some inconsistency it's missing?

we need TTL support, which currently explicitly marked as errUnsupported for some reason. Is this just a stub?

The TTL's are a stub too. A correct/efficient implementation needs some thought.

@yudai
Copy link
Contributor Author

yudai commented May 14, 2018

@heyitsanthony Thank you for the reference. I overlooked that readset and now I understand the behavior.

It's taking time more than I expected when I started this work. I'll make tests pass one by one anyway.

@stale
Copy link

stale bot commented Apr 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 6, 2020
@stale stale bot closed this Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants