Skip to content

k8s: pre-populate metadata cache. fixes https://github.com/tilt-dev/tilt/issues/3845#3854

Merged
nicks merged 1 commit into
masterfrom
nicks/ch9742
Oct 14, 2020
Merged

k8s: pre-populate metadata cache. fixes https://github.com/tilt-dev/tilt/issues/3845#3854
nicks merged 1 commit into
masterfrom
nicks/ch9742

Conversation

@nicks

@nicks nicks commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

Hello @landism,

Please review the following commits I made in branch nicks/ch9742:

4a1579a (2020-10-12 20:01:29 -0400)
k8s: pre-populate metadata cache. fixes #3845

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@nicks
nicks requested a review from landism October 13, 2020 00:03
@nicks
nicks force-pushed the nicks/ch9742 branch 3 times, most recently from 5b602c7 to 9f842e2 Compare October 13, 2020 00:55
@nicks

nicks commented Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

if the PR looks kind of intimidating, it's only really ~100 lines of code and 1,000 lines of tests, most of it very repetitive

@landism landism left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yay batches

do you think it's also worth getting rid of the last uses of K8sClient.dynamic and replacing it with K8sClient.meta, or has this PR taken care of all the throttling issues?

Comment thread internal/k8s/owner_fetcher.go Outdated
}

// As an optimization, we batch fetch all the ObjectMetas of a resource type
// on startup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe nitty, but this isn't at startup, is it? IIUC, it's the first time we check the objectmeta of any instance of a type, which I think is, like, the first time event watcher or pod watcher get updates? In either case, this part of the code is sufficiently removed from whether it happens at startup that the comment is hard to verify / keep up to date.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point! I reworded this

@nicks

nicks commented Oct 14, 2020

Copy link
Copy Markdown
Contributor Author

re: dynamic - i think we still need it for cases when we need the full resource data (pods, events, etc)

@nicks
nicks merged commit dceecf0 into master Oct 14, 2020
@nicks
nicks deleted the nicks/ch9742 branch October 14, 2020 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

throttling request regression

2 participants