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

[kbnServer/extensions] formalize request factories with helper #12697

Merged
merged 4 commits into from
Jul 11, 2017

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jul 7, 2017

With #12243 request's got a new request.getUiSettingsService() method that returned a new instance of the UiSettingsService that was already configured for that request based on the request context. This method wasn't terribly expensive, but since uiSettings are necessary in several places on almost every request the factory was cached per request; subsequent calls to request.getUiSettingsService() return the same return value as the first call.

This was implemented one-off in the specific request extension, but with #12625 we're adding another method to requests, request.getFieldFormatsService() that I think should have similar characteristics. Rather than re-implement the caching in a handful of places I figured we could formalize the pattern by creating a helper that future devs reading the code will be able to see and reuse for similar needs.

@spalger spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review v6.0.0 labels Jul 7, 2017
@epixa
Copy link
Contributor

epixa commented Jul 7, 2017

I think this may be premature. From a complexity standpoint, this is a 50 line abstraction to avoid repeating 4 lines of very-readable code, and it's a new abstraction in a request-oriented architecture that we don't necessarily plan to repeat in the new platform. Personally, I'd prefer that we explore an abstraction like this after we reuse the existing convention a couple times and realize it is actually causing us problems.

I could be swayed though, so I'm interested in what others think on this.

@kimjoar
Copy link
Contributor

kimjoar commented Jul 7, 2017

I'm not really sure what I think about this abstraction in general. All kinds of caching has a tendency to create subtle bugs. It also "hides complexity" in a way I'm not sure I like. The more I think about it the more I lean towards injecting the request (or the request specific object, like a request-scoped cluster) in the method being called within the request handler.

However, as mentioned, I'm not really sure what I think yet. I've got to explore these things a bit.

But that's something we can tackle in a more focused manner in the new platform work, so I'm okey with this for now, if others prefer this.

@azasypkin
Copy link
Member

I strongly agree with @kjbekkelund on general caching concern, but if we decide (decided?) that we really need caching in several different places, I'd rather prefer to handle it in a single place like in this PR.

From a complexity standpoint, this is a 50 line abstraction to avoid repeating 4 lines of very-readable code

Strictly speaking half of those 50 lines are jsdoc/comments that is great and doesn't increase complexity :) All in all adding of reasonable amount of complexity doesn't concern me too much as long as it's sufficiently covered with unit tests that seems to be the case here.

@spalger
Copy link
Contributor Author

spalger commented Jul 7, 2017

Talked with @kjbekkelund and @epixa offline who agreed we can move forward with this in master and we'll explore more desirable directions in the context of the new platform once we get to more in-depth http stuff there. So this is ready for review again

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

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

Just a nitpick. LGTM


const requestCache = new WeakMap();
server.decorate('request', methodName, function () {
if (!requestCache.has(this)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only way to do this? No other access to the request?

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, maybe const request = this above this line? Just to make it super-obvious.

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

@spalger spalger merged commit a081ec0 into elastic:master Jul 11, 2017
@spalger spalger deleted the server/formalize-request-factories branch October 18, 2019 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.0.0-rc1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants