Add routing header 1.5 #3865
Add routing header 1.5 #3865
Conversation
slinkydeveloper
force-pushed
the
add-routing-header-1.5
branch
from
ef52641 to
c2a5520
Compare
This lets you use a specific header as "discriminator" when registering a service.
slinkydeveloper
force-pushed
the
add-routing-header-1.5
branch
from
c2a5520 to
57ec551
Compare
|
cc @muhamadazmy this is ready for the first pass. |
|
Oh I just figured one little issue, fixing it |
…d was problematic, more in the code comment.
muhamadazmy
left a comment
muhamadazmy
left a comment
There was a problem hiding this comment.
Thank you for this PR @slinkydeveloper but I left few questions because I might have different understanding of how this should work. let me know if I missed something
| schemas.ty.protocol_type() == deployment_metadata.ty.protocol_type() | ||
| && schemas.ty.normalized_address() | ||
| == deployment_metadata.ty.normalized_address() | ||
| && routing_header.as_ref().is_none_or( |
There was a problem hiding this comment.
This means if a new deployment that has No routing header will conflict with a deployment that has a routing header?
is it possible that a single deployment (behind the LB) is routed to as "default" without any headers and multiple ones with a routing header?
If this is true, this means conflicting here is not correct.
There was a problem hiding this comment.
is it possible that a single deployment (behind the LB) is routed to as "default" without any headers and multiple ones with a routing header?
I didn't think about this as possibility, and right now I don't see much the use case for this to be fair. If an LB has a "default", probably routes to latest route, which is what you don't want to use I suppose. In the case of the customer we're trying to help, the default doesn't exist, they'll always provide an header. Hence the following registration sequence:
- localhost:9080 with routing header
- localhost:9080 without routing header -> this conflicts, and needs to be forced
Also, without storing the information that the routing_header is in fact a routing header, I don't see much alternative to this condition, right? I can't change the condition to routing_header.is_some_and, otherwise it breaks when not using this feature.
There was a problem hiding this comment.
What will happen if I did this instead:
- localhost:9080 without routing header (works)
- localhost:9080 with routing header (will also work) will not conflict.
So the system will behave differently by changing the order of registration.
What i think might work instead is to filter mainly like address == deployment.address && routing_header == deployment.routing_header
Accept the key of the routing header via configuration. This makes the behavior more consistent.
slinkydeveloper
force-pushed
the
add-routing-header-1.5
branch
from
7ee42be to
4680ec4
Compare
muhamadazmy
left a comment
muhamadazmy
left a comment
There was a problem hiding this comment.
LGTM! Thank you so much for bearing with me :)
|
Thanks for the tips! |
2 participants
Add new field in discovery
routing-headerthat partecipates in the semantic equality of deployments. The header is then just stored like all the other additional-headers, this feature is just an api behavioral addition.With this feature, users can register multiple HTTP deployments with the same URI, but with a different routing header.
TODO: