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

etcd v3 initial change #22604

Closed
wants to merge 3 commits into from
Closed

etcd v3 initial change #22604

wants to merge 3 commits into from

Conversation

hongchaodeng
Copy link
Contributor

This serves as a follow up on #22448.

The conversation is good! Let's keep it up while working out some real code.

I'm initiating a few things here:

  • bump up etcd godep. The etcd would support both V2 and V3 API for us to do the work. Minor fix is done on EtcdtestServer.
  • I set up the structure of etcd3 helper and implements Set(), Get().

@k8s-bot
Copy link

k8s-bot commented Mar 6, 2016

GCE e2e build/test failed for commit 21b84e806130aae5d328a98cde3aea80e1582c72.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot
Copy link

Labelling this PR as size/XS

@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 6, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 6, 2016

GCE e2e build/test failed for commit af90a13f0a14bc3ff8f17a787397bfa6ad6c27b7.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Mar 6, 2016

GCE e2e build/test failed for commit 3afddb73ad53e2d4bb8e741a9248792c556ecb3a.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Mar 6, 2016

GCE e2e build/test failed for commit 03719896063dce058eb90b60d59665810e116a2e.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@wojtek-t
Copy link
Member

wojtek-t commented Mar 7, 2016

@hongchaodeng - can you please move Godeps update to a separate PR? It would be so much easier to review this PR then, because github doesn't support well PRs with so many files...

Also, I assume that with upgraded Godeps, etcd v2 in current setup will still work, right?

@k8s-bot
Copy link

k8s-bot commented Mar 11, 2016

GCE e2e build/test failed for commit 03719896063dce058eb90b60d59665810e116a2e.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@timothysc
Copy link
Member

@hongchaodeng could you squash the helper commits please, I can review 2-day.

@wojtek-t
Copy link
Member

@timothysc - I didn't review this, just because we don't want to have it in 1.2. We are explicitly not merging big changes that are not going to be picked to 1.2 now.

@timothysc
Copy link
Member

@wojtek-t no worries, I had some time so just getting back into the fray, and wanted to keep an eye on this one.

@k8s-bot
Copy link

k8s-bot commented Mar 12, 2016

GCE e2e build/test failed for commit 66d6e033afe8f896ad8a575799a63cd3f5b850da.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Mar 12, 2016

GCE e2e build/test failed for commit d2be21e37b440455b03d36730e6d425e813a6f6e.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@hongchaodeng
Copy link
Contributor Author

I am constantly getting verify error:

output binaries contain bad symbols "testing[.]"
!!! Error in ./hack/../hack/verify-symbols.sh:28
  '"${KUBE_ROOT}/hack/after-build/verify-symbols.sh"' exited with status 1
Call stack:
  1: ./hack/../hack/verify-symbols.sh:28 main(...)
Exiting with status 1
FAILED   ./hack/../hack/verify-symbols.sh

@timothysc Can you help with that?

@k8s-bot
Copy link

k8s-bot commented Mar 12, 2016

GCE e2e build/test failed for commit 295749b5f50c3a6439e3f0c8e54c67ecb3c76820.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Mar 12, 2016

GCE e2e build/test failed for commit 21f089f27f94440ce6713630168f368c411cfaa1.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@wojtek-t
Copy link
Member

I've added few comments.

@hongchaodeng Can't we merge first two commits as a separate PR? We could do it now.

@hongchaodeng
Copy link
Contributor Author

@wojtek-t
Separate out the first two commits: #23194

Some deps are missing there because we only use them in v3 helper commits. But they are pretty minor.

@hongchaodeng
Copy link
Contributor Author

Add last commit "add custom storage error".
It abstracts away etcd-specific implementation of errors.
It's up to implementation re. how to parse error details and return storage errors.

/cc @xiang90 @wojtek-t @timothysc

@k8s-bot
Copy link

k8s-bot commented Mar 18, 2016

GCE e2e build/test passed for commit d94084710cb0de99de0d0ddf44306f43cff4a8f4.

@k8s-bot
Copy link

k8s-bot commented Mar 18, 2016

GCE e2e build/test passed for commit 39cfb67582b0b46f98d670fa2cd2141126e20861.

@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit 6ce4bb038b2541080fa70f823f4f3b82624122ef.

@@ -711,3 +710,21 @@ func (h *etcdHelper) addToCache(index uint64, obj runtime.Object) {
metrics.ObserveNewEntry()
}
}

func convertToStorageErrorIfPossible(err error, key string, rv uint64) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

just ToStorageErr?

Copy link
Contributor

Choose a reason for hiding this comment

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

is this an etcd specific func? or we might add other potential kv error types into switch cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"storage/" handles only four errors in special. See here.

Not sure about other error types ATM.

@k8s-github-robot
Copy link

@hongchaodeng PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2016
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 19, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit 97a1ed882ce2afaebbd79e81b0a26ac50dee6169.

@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit e0ee1e0846ad3120f10b6aa9c129340062af4a04.

@k8s-bot
Copy link

k8s-bot commented Mar 19, 2016

GCE e2e build/test passed for commit bc8a262.

@hongchaodeng
Copy link
Contributor Author

Rebased and addressed comments.

@hongchaodeng
Copy link
Contributor Author

I moved out the storage error commit into another PR (#23295) for easy review. PTAL.

@hongchaodeng
Copy link
Contributor Author

This PR is way too long.

It's github's problem -- They should have better ways to show long discussion.

Anyway, I'm gonna closing this. Once #23295 is merged, I will rebase and submit another PR with more KV methods and improvement of code quality.

@hongchaodeng
Copy link
Contributor Author

Created a new PR #23387

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet