Skip to content

Tracking resources refactoring#2027

Merged
knative-prow-robot merged 35 commits into
knative:masterfrom
nachocano:tracker
Oct 14, 2019
Merged

Tracking resources refactoring#2027
knative-prow-robot merged 35 commits into
knative:masterfrom
nachocano:tracker

Conversation

@nachocano
Copy link
Copy Markdown
Contributor

@nachocano nachocano commented Oct 10, 2019

Fixes #1998 and others unreported bugs (e.g., Source-Trigger dependency reported by @aaron-lerner)

Proposed Changes

  • Refactoring ResourceTracker to be ListableTracker, so that it can track any apis.Listable object.
  • Only using ListableTracker when we need to dynamically generate informers (e.g., Channelable, KResource, AddressableType). If we know the concrete time at compile time, then use the pkg tracker.
  • Avoid going to the API server every time we reconcile Channelables and Addressables. This change affects sequences, parallel, subscriptions, etc. Creating a Resource tracker used to work because we were always going to the API server instead of using the listers.
  • Not using tracker for things that we own and are typed at compile time (e.g., Deployment in ApiServerSource)

Pending

  • I think with this change we will be able to create the dynamic trackers only once and share them across controllers within the same process. We can do that as a follow up.

Release Note

NONE

@knative-prow-robot knative-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 10, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 10, 2019
@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Oct 10, 2019
@nachocano
Copy link
Copy Markdown
Contributor Author

/assign @Harwayne @grantr

not ready for review yet...

@nachocano nachocano changed the title [WIP] Updates to duck Tracker and its usage [WIP] Updates the duck Tracker and its usage Oct 10, 2019
@nachocano
Copy link
Copy Markdown
Contributor Author

/assign @vaikas-google

Comment thread pkg/reconciler/broker/broker.go Outdated
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 14, 2019
@nachocano
Copy link
Copy Markdown
Contributor Author

/hold need to do one more nil check

@Harwayne
Copy link
Copy Markdown
Contributor

/hold need to do one more nil check

/hold

(It needs to be on its own line.)

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2019
@nachocano
Copy link
Copy Markdown
Contributor Author

/hold need to do one more nil check

/hold

(It needs to be on its own line.)

Thanks!

/hold cancel

done the change...

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2019
@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Oct 14, 2019

/approve
Thanks for doing this! Leaving lgtm for @Harwayne and @lionelvillard in case they had any additional comments.

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2019
@lionelvillard
Copy link
Copy Markdown
Contributor

LGTM

Leaving the last LGTM to @Harwayne

Comment thread pkg/duck/listable.go Outdated
Comment thread pkg/reconciler/channel/channel.go Outdated
Comment thread pkg/reconciler/channel/channel.go Outdated
Comment thread pkg/reconciler/channel/channel.go Outdated
Comment thread pkg/reconciler/channel/channel.go Outdated
Comment thread pkg/reconciler/trigger/trigger.go Outdated
Comment thread pkg/reconciler/subscription/subscription.go Outdated
Comment thread pkg/reconciler/subscription/subscription.go Outdated
Comment thread pkg/reconciler/subscription/subscription.go Outdated
Comment thread pkg/reconciler/subscription/subscription.go Outdated
Comment thread pkg/reconciler/broker/broker.go Outdated
Comment thread pkg/reconciler/sequence/sequence.go Outdated
@nachocano
Copy link
Copy Markdown
Contributor Author

@Harwayne ready for another look

Comment thread pkg/reconciler/subscription/subscription.go Outdated
Copy link
Copy Markdown
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2019
@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/duck/listable.go Do not exist 68.9%
pkg/duck/subscriber.go 97.2% 83.7% -13.5
pkg/reconciler/apiserversource/apiserversource.go 77.6% 77.8% 0.2
pkg/reconciler/broker/broker.go 81.2% 81.3% 0.1
pkg/reconciler/channel/channel.go 81.9% 75.9% -6.0
pkg/reconciler/parallel/parallel.go 72.2% 71.4% -0.8
pkg/reconciler/sequence/sequence.go 71.6% 72.4% 0.9
pkg/reconciler/subscription/subscription.go 76.7% 75.9% -0.8
pkg/reconciler/trigger/trigger.go 85.2% 84.1% -1.1

@knative-prow-robot knative-prow-robot merged commit 48b3869 into knative:master Oct 14, 2019
@lionelvillard
Copy link
Copy Markdown
Contributor

@nachocano what about move listable.go over to knative.dev/pkg?

@nachocano
Copy link
Copy Markdown
Contributor Author

@nachocano what about move listable.go over to knative.dev/pkg?

+1 yes I think it will be useful. If you have some free cycles, go for it... Otherwise I may bee able to do it in the upcoming weeks. I think we should also move Channelable. We briefly discussed that with @n3wscott sometime back...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trigger doesn't notice Broker changes

8 participants