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

Generalized factory registration #1127

Merged
merged 5 commits into from
Jun 17, 2017
Merged

Conversation

mrice32
Copy link
Member

@mrice32 mrice32 commented Jun 16, 2017

This generalized registration interface should reduce the burden of introducing new types of implementations that are statically registered and looked up at runtime by name (currently what is used for filters and tracers) and simplify the interface. As mentioned in a previous modification to the filter registration mechanism, it may be possible to get this new interface introduced with no deprecation because the Named* filter factories and the tracer factory that this touches were introduced in this release cycle. #967

htuch
htuch previously approved these changes Jun 16, 2017
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Nice.

htuch
htuch previously approved these changes Jun 16, 2017
*/
static RegisterNamedHttpFilterConfigFactory<BufferFilterConfig> register_;
static Registry::RegisterFactory<BufferFilterConfig, NamedHttpFilterConfigFactory> register_;
Copy link
Member

Choose a reason for hiding this comment

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

Having just worked on this code, IIRC it's now possible to remove "server/config/network/http_connection_manager.h" from includes and bazel deps. Similarly for network filters IIRC you only need registry.h and filter_config.h and can remove configuration_impl.h/bazel dep. (I actually thought about doing something like this but got lazy, so this is awesome, thanks).

@mrice32
Copy link
Member Author

mrice32 commented Jun 16, 2017

If we agree that we don't want to deprecate the previous Named* factory registration mechanism because of its recent introduction, how should I update the envoy-filter-example repo to allow the CI to pass (I think that's the only failure left)? Should I just make a PR for the other repo, get it committed, and then increment the hash to match in this PR?

@mattklein123
Copy link
Member

@mrice32 you can look at what I did last time.

  1. Make new branch in filter example repo with fixes
  2. Point this change CI to SHA of that branch
  3. Commit this
  4. Merge that branch to master
  5. Do another envoy PR to repoint SHA to master.

It's annoying but doesn't happen very often.

@mattklein123 mattklein123 merged commit 0300f37 into envoyproxy:master Jun 17, 2017
@mrice32 mrice32 deleted the new_reg branch May 23, 2018 01:30
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
Automatic merge from submit-queue.

[DO NOT MERGE] Auto PR to update dependencies of proxy

This PR will be merged automatically once checks are successful.
```release-note
none
```
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: The high-level representation of filter statuses with attached HTTP entities to be forwarded can't be directly represented across the bridge layer and must be mapped. Uniquely, resume iteration statuses may contain pending entities that have not yet been forwarded. This quick fix checks bounds explicitly for these cases in Objective-C.
Risk Level: Low

Signed-off-by: Mike Schore <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: The high-level representation of filter statuses with attached HTTP entities to be forwarded can't be directly represented across the bridge layer and must be mapped. Uniquely, resume iteration statuses may contain pending entities that have not yet been forwarded. This quick fix checks bounds explicitly for these cases in Objective-C.
Risk Level: Low

Signed-off-by: Mike Schore <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.

3 participants