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.

Sign up for GitHub

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

Jump to bottom

Pass SelectionPredicate instead of Filter to storage layer #31190

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Pass SelectionPredicate instead of Filter to storage layer #31190

merged 1 commit into from
Sep 26, 2016

Conversation

hongchaodeng
Copy link
Contributor

@hongchaodeng hongchaodeng commented Aug 22, 2016 •
edited by jessfraz
Loading

Depends on #31189 (first commit).

ref: #29888

What?
This PR removes the filtering logic and passes SelectionPredicate to storage layer.

Why?
Filter doesn't provide enough information of and isn't the right abstraction for indexing. We need to pass in SelectionPredicate instead.

etcd3: pass SelectionPredicate instead of Filter to storage layer

This change is Reviewable

@hongchaodeng
Copy link
Contributor Author

/cc @lavalamp

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Aug 23, 2016
@hongchaodeng hongchaodeng force-pushed the r2 branch 2 times, most recently from fa74ed9 to b7bce14 Compare August 23, 2016 04:37
@hongchaodeng
Copy link
Contributor Author

All tests passed

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 23, 2016
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2016
@hongchaodeng
Copy link
Contributor Author

@lavalamp
Can you help review this PR? The first commit is from #31189 and Clayton has given "lgtm". The second commit is the change of passing SelectionPredicate.
We would like to iterate on review feedback early.

@hongchaodeng
Copy link
Contributor Author

"jenkins verif" failure non-relevant:

fatal: unable to access 'https://github.com/mesos/mesos-go/': Failed to connect to github.com port 443: Connection timed out
godep: error downloading dep (github.com/mesos/mesos-go/auth):

@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2016
@hongchaodeng
Copy link
Contributor Author

@k8s-bot e2e test this issue: #IGNORE

@hongchaodeng
Copy link
Contributor Author

@lavalamp
Can you help review this PR?
The first commit is from #31189 which has "lgtm" already.
The second commit is the change of passing SelectionPredicate.

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

k8s-bot commented Sep 8, 2016

GCE e2e build/test passed for commit da279a8bb90354db8a1b490ae9a840655683fcef.

@hongchaodeng
Copy link
Contributor Author

@lavalamp
Can you review this PR?

@calebamiles
Copy link
Contributor

calebamiles commented Sep 13, 2016 •
edited
Loading

@pwittrock: as this continues the work started in #31189, do you think it's reasonable to add a 1.5 milestone label on this PR so we

  • don't lose track of it when 1.5 coding opens 19 September
  • signal that this PR should only be reviewed once the cycles occupied by 1.4 are free

sorry for taking up a cycle myself

@xiang90 xiang90 added this to the v1.5 milestone Sep 13, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 4ef5d8cf2448a24681af3cc69415f76ea4d27be2.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@hongchaodeng
Copy link
Contributor Author

@lavalamp
Thanks for your feedback.
Addressed the comments:

  • remove generic.SelectionPredicate
  • Add todo for intermediate FilterFunc

PTAL. Will squash at the end.

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

Jenkins GCI Kubemark GCE e2e failed for commit 6f3ac80. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark gci e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@hongchaodeng
Copy link
Contributor Author

@k8s-bot kubemark gci e2e test this issue: #33388

@hongchaodeng
Copy link
Contributor Author

rebased and tests passed.
@lavalamp PTAL. Rebasing quite a few times.

@lavalamp
Copy link
Member

LGTM

@lavalamp lavalamp added release-note-none Denotes a PR that doesn't merit a release note. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Sep 26, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 987aef1 into kubernetes:master Sep 26, 2016
@hongchaodeng hongchaodeng deleted the r2 branch September 26, 2016 21:10
@wojtek-t wojtek-t modified the milestones: v1.4, v1.5 Oct 19, 2016
@wojtek-t wojtek-t mentioned this pull request Oct 20, 2016
Merged
@jessfraz jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 20, 2016
k8s-github-robot pushed a commit that referenced this pull request Oct 21, 2016
Merge pull request #35205 from wojtek-t/automated-cherry-pick-of-#31189-
bdbe2e1 
#31704-#32831-#32907-#33003-#33349-#31190-#33581-#34089-#34234-#32822-#33393-#34246-#34435-#32477-upstream-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 upstream release 1.4

We are going to release etcd v3 in 1.4.x release.

```
Cherrypick etcd v3-related bug fixes to 1.4 branch
```

Those PRs include:
- Updates of etcd Godeps
- Update to pkg/storage/etcd directory
- Two PR that were unnecessary to avoid conflicts: #31189 #31704

Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477

@hongchaodeng @xiang90 @lavalamp @smarterclayton
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#35205 from wojtek-t/automated-cherry-pi…
711c6c6 
…ck-of-#31189-kubernetes#31704-kubernetes#32831-kubernetes#32907-kubernetes#33003-kubernetes#33349-kubernetes#31190-kubernetes#33581-kubernetes#34089-kubernetes#34234-kubernetes#32822-kubernetes#33393-kubernetes#34246-kubernetes#34435-kubernetes#32477-upstream-release-1.4

Automatic merge from submit-queue

Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477 upstream release 1.4

We are going to release etcd v3 in 1.4.x release.

```
Cherrypick etcd v3-related bug fixes to 1.4 branch
```

Those PRs include:
- Updates of etcd Godeps
- Update to pkg/storage/etcd directory
- Two PR that were unnecessary to avoid conflicts: kubernetes#31189 kubernetes#31704

Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477

@hongchaodeng @xiang90 @lavalamp @smarterclayton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

Assignees

@lavalamp lavalamp

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.