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

[Management] Provide a way to fetch index pattern titles #13030

Merged

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Jul 21, 2017

Summary: This PR adds another method to the indexPatterns service to fetch index patterns titles. Since the code was nearly identical to how index pattern ids are fetched, I decided to slightly refactor the function to accept a parameter for which property to fetch from the index pattern. I've updated all usages to the new approach.

Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

Open question.

cachedPromise = null;
return (field) => {
const getter = get.bind(get, field);
getter.clearCache = function () {
Copy link
Member

Choose a reason for hiding this comment

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

Previously the IndexPatternsProvider would clear the complete cache whenever it was invalidated. With this change, only the part of the cache used by the respective user is cleared. This seems like a dangerous way to end up with everything sitting in memory forever.

I wonder if we should only cache IDs, and simply fetch other things on demand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup great point. I'm honestly not sure if caching will provide much benefit as it's not clear how consumers will use this for non-ids, but it made sense to me (at the time) to keep the behavior consistent. The previous callers to clearCache were only clearing cache around ids so it felt odd to clear the entire cache, but that might be the right call, or as you eluded to, maybe just not bother caching non-ids requests entirely.

I'm open to changing it.

@chrisronline
Copy link
Contributor Author

@pickypg Went ahead and made that change

Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

LGTM. I'm not that familiar with this code though. It would be nice to have some tests though?

@BigFunger
Copy link
Contributor

LGTM

@chrisronline chrisronline merged commit 89c7456 into elastic:master Jul 24, 2017
chrisronline added a commit that referenced this pull request Jul 24, 2017
* Refactor how `get_ids` works to enable fetching other properties of index patterns, specifically the `title` or name

* Remove unnecessary lines

* Only cache for `id` calls
chrisronline added a commit that referenced this pull request Jul 24, 2017
* Refactor how `get_ids` works to enable fetching other properties of index patterns, specifically the `title` or name

* Remove unnecessary lines

* Only cache for `id` calls
@chrisronline
Copy link
Contributor Author

chrisronline commented Jul 24, 2017

Backport:

5.6: 78ac8e7
6.0: 0b66012
6.x: eb9f8a5

chrisronline added a commit that referenced this pull request Aug 2, 2017
* Refactor how `get_ids` works to enable fetching other properties of index patterns, specifically the `title` or name

* Remove unnecessary lines

* Only cache for `id` calls
@chrisronline chrisronline deleted the enhancement/get_index_pattern_titles branch August 10, 2017 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants