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
feat: allows users to specify plugin execution priority #7273
feat: allows users to specify plugin execution priority #7273
Changes from all commits
b3e59c1
46ca8b2
6592f6b
92b945d
600a8db
945466b
7fb05cc
f417d87
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.
As I already mentioned, this is poor Developer Experience. It makes the use of the configuration more complex for no reason except that it's easier to implement.
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 also think
priority
should not be in_meta
, it doesn't fit the definition of_meta
, it only works for plugin bound objects. How about a custom priority directly as a base property of the plugin? ref: #6580 (comment)cc @spacewander @membphis
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.
The plugin property will affect every plugin configuration, not just what we configure for.
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.
After discussing with @spacewander , I changed my mind.
_meta
is based on the concept of abstraction of all plugin configurations, which are available to all plugins. So to distinguish it, it should have a separate configuration field, i.e._meta
.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.
Sorry, but again, you speak from an engineering perspective. I'm talking from a user perspective. As a user, it's not fundamental whether the configuration parameter can be used across multiple plugins.
Here, I need to know and remember:
priority
meta
_
Only #1 is mandatory. You make #2 and #3 necessary and impair the ease of use for engineering reasons. While I understand there's a tension between the ease of implementation and ease of use, I don't see any good reason to force those additional requirements onto users at this point.
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.
Yes, it is most directly to make
priority
one of the properties of the plugin. just likedisable
, what your option? @spacewanderThere 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.
No. Setting the new field directly on the configuration may override people's current configuration. We should avoid break change
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 have submitted a PR to explain it in the comment: #7290
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.
At this point, why do we keep two different functions? We can keep a single one as call it whenever we required it. Can we change the sample?
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.
A good use-case would be the combination of
proxy-mirror
plugin and theproxy-rewrite
one.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 tried it and the two plugins don't show well to work together by customizing the priority.
Maybe you're talking about changing the request path via the
proxy-rewrite
plugin, and thenproxy-mirror
using that new path.But
proxy-rewrite
plugin update uri on the varctx.var.upstream_uri
, butproxy-mirror
get the uri by the varctx.var.uri
, this is the difference in implementation.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.
That's actually the origin of my initial requirement.
Indeed!
Imagine that I receive a request for
v1/hello
. Now, my requirements are:v1
to be removedAgain, how are users supposed to know this? It's an implementation detail.
The requirements make sense from a functional point of view.
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
proxy-mirror
support rewritepath
, so we can implement the above requirements, but we just need to configure them separately onproxy-mirror
andproxy-rewrite
.Of course, we can also modify the
proxy-mirror
plugin to read thectx.var.upstream_uri
variable. But it won't be done in this PR.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.
in this way, we can match the requirements