proposal icon indicating copy to clipboard operation
proposal copied to clipboard

A41: Clarify metadata match when invert is set

Open yashykt opened this issue 4 years ago • 11 comments

yashykt avatar Jan 14 '22 19:01 yashykt

Oh, you're talking about MetadataMatcher.invert. That is a new field that did not exist when this gRFC was made (looks like it was added a few days before the gRFC was finalized), and that I wasn't aware of.

Maybe we just document that it did not exist for the gRFC?

ejona86 avatar Jan 18 '22 22:01 ejona86

Updated to note that Java and Go do not support MetadataMatcher.invert as of yet.

yashykt avatar Jan 19 '22 19:01 yashykt

Any updates here? Go is waiting for this to be merged before implementing the behavior. (And if we do, maybe we should remove that Go/Java doesn't support the feature from the gRFC).

menghanl avatar Mar 16 '22 18:03 menghanl

I'm not wild about this being a moving target. Again, I think we should just say the point in time this was done and imply nothing about new fields.

@menghanl, why is Go adding/waiting for invert?

ejona86 avatar Mar 16 '22 20:03 ejona86

We have an issue tracking this: https://github.com/grpc/grpc-go/issues/5150 And @zasweq was waiting for this gRFC to be merged.

menghanl avatar Mar 16 '22 23:03 menghanl

That go issue sounds like, "hey it was added to the gRFC, let's add it to Go." I don't think it should be "added" to the gRFC. It came after the gRFC. The gRFC has nothing to do with it. We may say, "hey, by the way, there was stuff added to the proto after this that is not part of the gRFC" which helps understand the design. But that's different.

I think there have been some other discussions about when we'll add new fields, especially with matchers. This seems related. It should be discussed places other than here, I think.

ejona86 avatar Mar 16 '22 23:03 ejona86

What is this PR waiting for to be merged? Or do we not want to update the gRFC? If not, what documentation do we include this in? Just the proto file's comments?

dfawley avatar Apr 05 '22 17:04 dfawley

Based on @ejona86 's review, I guess this PR should be closed, or rather, we probably just mention the time/commit for which this gRFC is relevant

yashykt avatar Apr 05 '22 18:04 yashykt

Also, I don't think https://github.com/grpc/grpc-go/issues/5150 needs to be blocked on this

yashykt avatar Apr 05 '22 18:04 yashykt

Also, I don't think https://github.com/grpc/grpc-go/issues/5150 needs to be blocked on this

I agree. We will go ahead and implement this. What's the status in the other languages?

dfawley avatar Apr 05 '22 19:04 dfawley

I agree. We will go ahead and implement this. What's the status in the other languages?

Handled in Core

yashykt avatar Apr 05 '22 20:04 yashykt