Conversation
44f145a to
8286859
Compare
|
/cc @yanmxa |
376249c to
ca8841e
Compare
protocol/grpc/write_message.go
Outdated
| if value == nil { | ||
| delete(b.Attributes, contenttype) | ||
| } | ||
| b.Attributes[contenttype], _ = attributeFor(value) |
There was a problem hiding this comment.
Why do we delete the item from the map just to then re-add it here with nil ? Seems like this should be under an else or just remove the if+delete on lines 126-128. Same for below
There was a problem hiding this comment.
good catch, added else to handle non-nil case.
| return err | ||
| } | ||
| } | ||
| } else if name == contenttype { |
There was a problem hiding this comment.
what if there's both 'contenttype' and 'ce-datacontenttype' ?
There was a problem hiding this comment.
'ce-datacontenttype' will be handled by:
sdk-go/protocol/grpc/message.go
Lines 122 to 128 in ab07228
There was a problem hiding this comment.
yes but which one wins? Should we require them to be the same (if both are there)?
There was a problem hiding this comment.
When both 'ce-datacontenttype' and 'contenttype' are set, 'ce-datacontenttype' takes precedence. In practical scenarios, only 'contenttype' is necessary. Check how the 'contenttype' attribute is configured here:
sdk-go/protocol/grpc/write_message.go
Lines 121 to 130 in 6523960
This follow the similar approach that the Kafka protocol and PubSub protocol take, refer to the implementations in the CloudEvents SDK for Kafka (https://github.com/cloudevents/sdk-go/blob/main/protocol/kafka_sarama/v2/write_producer_message.go#L103-L114) and PubSub (https://github.com/cloudevents/sdk-go/blob/main/protocol/pubsub/v2/write_pubsub_message.go#L70-L80).
protocol/grpc/write_message.go
Outdated
| delete(b.Attributes, prefix+time) | ||
| } | ||
| b.Attributes[prefix+time], _ = attributeFor(value) | ||
| return nil |
There was a problem hiding this comment.
Nit: you can remove all of these "return nil" lines since it'll happen due to the return on line 156
protocol/grpc/write_message.go
Outdated
|
|
||
| b.Attributes[prefix+name], err = attributeFor(value) | ||
|
|
||
| return |
There was a problem hiding this comment.
Let's define 'err' in the func instead of in the return arg - just for consistency
|
I see we have a sample, but can we get some testcases too? |
|
unit tests are failing |
ca8841e to
d4ad609
Compare
ab07228 to
f719045
Compare
|
@morvencao can you take a look at the failing checks? |
|
@duglin I've resolved the issues with testing and successfully executed it on my local environment, passing all tests. |
|
|
||
| import ( | ||
| any "github.com/golang/protobuf/ptypes/any" | ||
| any1 "github.com/golang/protobuf/ptypes/any" |
There was a problem hiding this comment.
Why replace any with any1? Is this intentional?
There was a problem hiding this comment.
This code is generated by proto compiler, not manually altered.
There was a problem hiding this comment.
I suspect it does this since any is now a data type in go
| // CloudEventServiceClient is the client API for CloudEventService service. | ||
| // | ||
| // For semantics around ctx use and closing/ending streaming RPCs, please refer to https://pkg.go.dev/google.golang.org/grpc/?tab=doc#ClientConn.NewStream. | ||
| type CloudEventServiceClient interface { |
There was a problem hiding this comment.
The Publish-subscribe model has the characteristics: The publisher sends events without specifically targeting any recipient. Subscribers express interest in certain types of messages and receive notifications or updates when messages of interest are published.
In my opinion, the gRPC does not explicitly refer to the publish-subscribe model. Instead, it resembles more of a request-response model, like the HTTP protocol. So I suggest aligning these interfaces with the http protocol.
| @@ -1 +0,0 @@ | |||
| mode: atomic | |||
There was a problem hiding this comment.
I did delete the file. but I'm unsure if we should do that.
Based on the UT script at https://github.com/cloudevents/sdk-go/blob/main/hack/unit-test.sh#L20, it seems that coverage.tmp is produced as intermediate output by UT. Deleting it should pose no harm.
There was a problem hiding this comment.
I can restore it if there are additional uses that I overlooked.
4ea25db to
8f12cd7
Compare
|
fixed the testing and rebased the code. |
|
rebase is needed |
Signed-off-by: morvencao <lcao@redhat.com>
Signed-off-by: morvencao <lcao@redhat.com>
Signed-off-by: morvencao <lcao@redhat.com>
Signed-off-by: morvencao <lcao@redhat.com>
Signed-off-by: morvencao <lcao@redhat.com>
Signed-off-by: morvencao <lcao@redhat.com>
8f12cd7 to
9dcecb3
Compare
|
ping @morvencao |
resolve: #980