Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RFC] Handler interface #36509
[RFC] Handler interface #36509
Changes from all commits
2b95f61
5ac8665
13427ac
5e4fab9
18e1301
571d609
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's an implementation detail, but imo important don't forget to make context extension lazy (as a getter?) and execute this function every time when someone reads from
context.[extension point]
otherwise context may have stale version of the client.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think context needs to be generated immediately before invoking the handler. The idea is that context does not change once its corresponding handler begins executing, so even if the ES client were to update at the service level, a handler that has already started executing would continue to use the original client until it finishes executing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshdover We should call this out specifically in the RFC because it's an important detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this RFC is about a broad concept rather than a specific feature, I'm fine proceeding with this RFC as-is, but this detail is an important problem to address for implementations. Realistically, I think this means that core must provide plugin developers a mechanism to build a context since by nature a plugin won't even know about plugins that it does not depend on, so they can't properly construct the context themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We may actually need to expose a service to plugins that allows them to offer context registration and construction. This service would handle the dependency management aspect of constructing context.
This needs to be built before any plugins begin offering context in their handlers.