-
Notifications
You must be signed in to change notification settings - Fork 591
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
Forwarding OPTIONAL attributes #461
Conversation
@deissnerk I think the CI issues are real |
Good clarification, LGTM |
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.
👍 for clarifying this!
spec.md
Outdated
include that feature in a message if it wants, and a consumer can choose to | ||
support that feature if it wants. A consumer that does not support that feature | ||
will then silently ignore that part of the message. The producer needs to be | ||
prepared for the situation where a consumer ignores that feature. For an |
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 know this isn't part of your PR, but I'm struggling with what this sentence is trying to say.
The producer should be loosely coupled from the consumer anyway. That makes the producer always prepared for the consumer ignoring parts of the message (whether OPTIONAL or not) or the whole message, doesn't it? 😉
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 you already said, I didn't write this. 😃
Could it mean that without the OPTIONAL attribute the consumer would interpret the event differently?
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.
Normally that kind of language is important when the specs have a processing model associated with it. In those cases a REQUIRED property typically can not be ignored by the receiver if they want to be compliant with the spec because there's some processing rules associated with it. While an OPTIONAL property can be ignored, which means the sender needs to be prepared for that.
In our case, we don't have anything as strong as a processing model that people need to adhere to, but I do think people need to be prepared for cases such as when they add "partionkey" to events and assume that means the receiver will put all incoming events into the same partition, but in fact that may not happen. The receiver can completely ignore it.
spec.md
Outdated
will then silently ignore that part of the message. The producer needs to be | ||
prepared for the situation where a consumer ignores that feature. For an | ||
[Intermediary](#intermediary) silently ignoring OPTIONAL attributes means that | ||
it MUST forward them. |
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'm struggling with "silently" ignoring (also above). How would a one (consumer or intermediary) "loudly" ignore an attribute? If the intermediary "loudly" ignores the attribute, then the MUST doesn't apply?
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.
Proposal:
An Intermediary MUST forward OPTIONAL attributes.
or, less strict:
An Intermediary MUST forward OPTIONAL attributes, unless explicitly configured to ignore them.
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.
Well, loudly ignoring to me usually means that it logs messages starting with "WARNING" or "ERROR" which then might cause some indicator on an operator's dashboard show a red or yellow light. 😀
I would also be open to your less strict proposal, because that is what I had in mind. I don't think you can foresee all use cases for intermediaries. Perhaps there will be use cases where intermediaries are used to filter out certain attributes. Basically, I wanted to express that an intermediary should only drop an attribute, if it is INTENTIONALLY, but using the word intention in combination with a piece of software seemed strange. 😉
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.
LOL "loudly ignore" - clearly it's been a while since you've gotten the 'silent treatment' !
Same as @deissnerk, to me "silently ignore" it means they shouldn't do something like reject the request because of it.
I'm kind of wondering what "explicitly configured" means though... does this mean that if someone "explicitly configures" a consumer to ignore (not forward) all optional attributes they're ok?
For reference, RFC2068 says this:
The extension-header mechanism allows additional entity-header fields
to be defined without changing the protocol, but these fields cannot
be assumed to be recognizable by the recipient. Unrecognized header
fields SHOULD be ignored by the recipient and forwarded by proxies.
and
Hop-by-hop headers, which are meaningful only for a single
transport-level connection, and are not stored by caches or
forwarded by proxies.
I think the use of "unless explicitly configured" is a potentially confusing way of saying SHOULD or STRONGLY RECOMMENDED, so I would prefer to avoid any potential confusion and just use one of those instead of MUST.
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.
@duglin I liked the notion of silently ignoring, because it suggests that if an intermediary does not know what to do with it, it MUST just pass it on. In the end it boils down to "pass it on, unless you know exactly what you are doing". So your proposal is fine to me, as well.
The example with the hop-by-hop headers is interesting. When we were still discussing property bags, @clemensv was proposing something similar to the AMQP message annotations for this. In the AMQP spec you find:
The message-annotations section is used for properties of the message which are aimed at the infrastructure and SHOULD be propagated across every delivery step. Message annotations convey information about the message. Intermediaries MUST propagate the annotations unless the annotations are explicitly augmented or modified
So there the term explicitly is used.
Approved on the 6/20 call - but use SHOULD in there instead of MUST @deissnerk let me know when it's ready and I'll merge it |
…e them. Signed-off-by: Klaus Deissner <[email protected]>
Signed-off-by: Klaus Deissner <[email protected]>
Signed-off-by: Klaus Deissner <[email protected]>
Signed-off-by: Klaus Deissner <[email protected]>
LGTM |
LGTM 👍 |
@cneijenhuis thanks. merging per approval on last week's call. |
Addresses issue #290.