make controller variables available to kafka trigger #41
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event id was always present. Are you suggesting it is redundant?
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.
Yes, but that is @andresmgot call 🤷♂️
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, let's avoid breaking changes if possible. Some functions may rely in the current format.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: