make controller variables available to kafka trigger #41
Conversation
Co-authored-by: Barak Shechter <barak.shechter@gmail.com>
Co-authored-by: Barak Shechter <barak.shechter@gmail.com>
Co-authored-by: Barak Shechter <barak.shechter@gmail.com>
| @@ -56,10 +59,16 @@ func GetHTTPReq(funcName string, funcPort int, kafkaTopic, namespace, eventNames | |||
| if err != nil { | |||
| return nil, fmt.Errorf("Failed to create a event-ID %v", err) | |||
| } | |||
| messageTimestamp := kafkaMessage.Timestamp.UTC().Format(time.RFC3339) | |||
| req.Header.Add("event-id", eventID) | |||
There was a problem hiding this comment.
I am not sure what is the intention of having a n id, which has nothing to do with the message. It's not a X-Request-ID. Should we remove it?
There was a problem hiding this comment.
The event id was always present. Are you suggesting it is redundant?
There was a problem hiding this comment.
Yes, but that is @andresmgot call 🤷♂️
There was a problem hiding this comment.
The goal of the eventID was to simply have a unique ID for the message. I would leave it (regardless of its usefulness) to avoid breaking changes.
| @@ -56,10 +59,16 @@ func GetHTTPReq(funcName string, funcPort int, kafkaTopic, namespace, eventNames | |||
| if err != nil { | |||
| return nil, fmt.Errorf("Failed to create a event-ID %v", err) | |||
| } | |||
| messageTimestamp := kafkaMessage.Timestamp.UTC().Format(time.RFC3339) | |||
| req.Header.Add("event-id", eventID) | |||
There was a problem hiding this comment.
Yes, but that is @andresmgot call 🤷♂️
| @@ -59,7 +62,11 @@ func GetHTTPReq(funcName string, funcPort int, kafkaTopic, namespace, eventNames | |||
| req.Header.Add("event-id", eventID) | |||
| req.Header.Add("event-time", timestamp.String()) | |||
There was a problem hiding this comment.
I think I was misunderstood. What I had in mind is to use the message timestamp instead of the current time
| req.Header.Add("event-time", timestamp.String()) | |
| req.Header.Add("event-time", kafkaMessage.Timestamp.UTC().Format(time.RFC3339)) |
There was a problem hiding this comment.
@sepetrov re event-id and event-time: removing or changing the values would be changing the meaning of those fields and represent a breaking change
There was a problem hiding this comment.
by adding the more relevant fields and new headers, we keep backwards compat but start putting in more useful info
There was a problem hiding this comment.
That is true. My suggestions are BC-breaking, because I personally to see any value in current implementation. That is @andresmgot call again. I would be happy to have the new metadata available regardless the name.
There was a problem hiding this comment.
yes, let's avoid breaking changes if possible. Some functions may rely in the current format.
There was a problem hiding this comment.
Thanks for the PR @barakshechter! The code looks good to me but you need to fix the format issues.
| kubelessutils "github.com/kubeless/kubeless/pkg/utils" | ||
| "github.com/sirupsen/logrus" | ||
| "github.com/spf13/cobra" | ||
| "github.com/kubeless/kafka-trigger/pkg/controller" |
There was a problem hiding this comment.
I believe these changes in the imports order causes a gofmt issue, you'd need to set it back:
These files are not properly gofmt'd:
- cmd/kafka-trigger-controller/kafka-controller.go
- pkg/controller/kafka_trigger_controller.go
| @@ -56,10 +59,16 @@ func GetHTTPReq(funcName string, funcPort int, kafkaTopic, namespace, eventNames | |||
| if err != nil { | |||
| return nil, fmt.Errorf("Failed to create a event-ID %v", err) | |||
| } | |||
| messageTimestamp := kafkaMessage.Timestamp.UTC().Format(time.RFC3339) | |||
| req.Header.Add("event-id", eventID) | |||
There was a problem hiding this comment.
The goal of the eventID was to simply have a unique ID for the message. I would leave it (regardless of its usefulness) to avoid breaking changes.
| @@ -59,7 +62,11 @@ func GetHTTPReq(funcName string, funcPort int, kafkaTopic, namespace, eventNames | |||
| req.Header.Add("event-id", eventID) | |||
| req.Header.Add("event-time", timestamp.String()) | |||
There was a problem hiding this comment.
yes, let's avoid breaking changes if possible. Some functions may rely in the current format.
Issue Ref: [Issue number related to this PR or None]
Description:
Make objects available to kafka controller, visible in events for consumption by functions
[PR Description]
TODOs: