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

api storage: Decouple Decorator from Filter #31189

Merged
merged 1 commit into from
Sep 8, 2016
Merged

api storage: Decouple Decorator from Filter #31189

merged 1 commit into from
Sep 8, 2016
+203 −14

Conversation

hongchaodeng
Copy link
Contributor

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

Continue #28249

What?
This PR decouples Decorator from Filter, i.e. remove Decorator in createFilter().

  • For List, Decorator is called on returned list object.
  • For Watch, we implement a new watcher to pipe through decorator. Error will be returned as a watch event.

Why?

  • We want to change filter to SelectionPredicate struct. But Decorator is designed to be coupled with filtering.
  • Per the discussion in registry store: remove unused Decorator #28249, decorator shouldn't be coupled to filter and error from Decorator should be returned instead of assuming false filtering.
etcd3: API storage - decouple decorator from filter

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 22, 2016
@hongchaodeng hongchaodeng mentioned this pull request Aug 22, 2016
Merged
@hongchaodeng
Copy link
Contributor Author

@smarterclayton
Tests passed. Ready for review.

smarterclayton
smarterclayton reviewed Aug 24, 2016
View reviewed changes
pkg/registry/generic/registry/store.go
return nil, err
}
if e.Decorator != nil {
if err := e.Decorator(out); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the behavior of the decorator - probably ok, but something that we need to call out on the decorator Godoc (gets passed either an individual object or the list type returned by the store).

@hongchaodeng
Copy link
Contributor Author

@smarterclayton
Updated docs. See last commit

@hongchaodeng
Copy link
Contributor Author

@smarterclayton
Fixed minor issue. Any further review? I will squash commits later.

@smarterclayton
Copy link
Contributor

smarterclayton commented Aug 25, 2016 •
edited
Loading

Lgtm please squash.

@miminar we'll have to update InageStrean decorators on the next rebase

@hongchaodeng
decouple Decorator from filter:
f01bc0a 
- Error from Decorator will be returned instead of assuming false filtering
- For List, Decorator is called on return list object
- For Watch, we implement a new watcher to pipe through decorator
@hongchaodeng
Copy link
Contributor Author

@smarterclayton
Squashed. Thanks for the review!

@hongchaodeng
Copy link
Contributor Author

@k8s-bot test this issue: #29149

@hongchaodeng
Copy link
Contributor Author

@smarterclayton
All tests passed

@smarterclayton smarterclayton added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 25, 2016
@xiang90 xiang90 added this to the v1.4 milestone Aug 25, 2016
@pwittrock
Copy link
Member

@xiang90 @smarterclayton Does this belong in the 1.4 milestone? Is it a feature or bug fix?

@xiang90
Copy link
Contributor

xiang90 commented Aug 25, 2016

@pwittrock

I assume it does. do not want to block other people's rebase. see: #31189 (comment)

Otherwise, you can remove it from the 1.4 milestone.

@pwittrock
Copy link
Member

Sorry I still don't totally understand. For context, I am the 1.4 release czar and working to stabilize the release. What depends on rebasing on top of this? Does this introduce new functionality?

I added the do-not-merge label until I better understand. Will remove if this still makes sense for 1.4

@pwittrock pwittrock added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 25, 2016
@xiang90
Copy link
Contributor

xiang90 commented Aug 25, 2016

@pwittrock @smarterclayton knows better for that issue.

One quick question: we stop merging in stuff for 1.4 unrelated things for now?

@pwittrock
Copy link
Member

@xiang90 That is correct. We are in feature freeze mode, and our intention is to only merge changes for bug fixes, tests, and docs. If you feel that you have a compelling reason to request for a 1 time exemption, there is a process for that, but something we are discouraging unless it is critical.

@xiang90
Copy link
Contributor

xiang90 commented Aug 25, 2016

@pwittrock Thank you.

@pwittrock
Copy link
Member

@smarterclayton If you have no objection, I plan to move this out of the v1.4 release

@smarterclayton
Copy link
Contributor

smarterclayton commented Aug 25, 2016 via email

@pwittrock pwittrock modified the milestones: v1.5, v1.4 Aug 25, 2016
@pwittrock pwittrock removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Aug 25, 2016
@pwittrock
Copy link
Member

@smarterclayton Thanks

@hongchaodeng
Copy link
Contributor Author

@k8s-bot test this issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Sep 7, 2016

GCE e2e build/test passed for commit f01bc0a.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Sep 8, 2016

GCE e2e build/test passed for commit f01bc0a.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit c27326a into kubernetes:master Sep 8, 2016
@hongchaodeng hongchaodeng deleted the r1 branch September 8, 2016 16:06
k8s-github-robot pushed a commit that referenced this pull request Sep 26, 2016
Merge pull request #31190 from hongchaodeng/r2
987aef1 
Automatic merge from submit-queue

Pass SelectionPredicate instead of Filter to storage layer

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.
@wojtek-t wojtek-t modified the milestones: v1.4, v1.5 Oct 19, 2016
This was referenced Oct 19, 2016
Closed
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
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
No reviews
Assignees

@smarterclayton smarterclayton

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
9 participants
@hongchaodeng @smarterclayton @pwittrock @xiang90 @k8s-bot @k8s-github-robot @jessfraz @googlebot @wojtek-t