Skip to content
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

🌱 add sequence id for status update event #308

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

skeeey
Copy link
Member

@skeeey skeeey commented Jan 2, 2024

Summary

Related issue(s)

Fixes #

@skeeey skeeey changed the title 🌱 add event sequence number 🌱 add event sequence id Jan 2, 2024
@skeeey
Copy link
Member Author

skeeey commented Jan 2, 2024

/assign @qiujian16

@skeeey
Copy link
Member Author

skeeey commented Jan 2, 2024

/assign @morvencao

@@ -29,12 +30,14 @@ const (

// ManifestCodec is a codec to encode/decode a ManifestWork/cloudevent with ManifestBundle for an agent.
type ManifestCodec struct {
restMapper meta.RESTMapper
sequenceGenerator *snowflake.Node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is only needed for status update, right? I do not think a developer of consumer need to care about this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, the agent generates the sequence id only when it creates the status update event.

and creating the snowflake generator when the agent init

@@ -48,6 +48,10 @@ const (
// ExtensionResourceVersion is the cloud event extension key of the resource version.
ExtensionResourceVersion = "resourceversion"

// ExtensionSequenceID is the cloud event extension key of the event sequence ID.
// The event sequence id represents the order in which events occur on the agent/source.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not the strict order. I think you can only use it to order the event from one single agent. I do not think you need this for source.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the source does not need this, it already has resoruceversion, update the description and variable name to make this more clear

@skeeey skeeey changed the title 🌱 add event sequence id 🌱 add sequence id for status update event Jan 2, 2024

func init() {
// init the snowflake id generator, for each single agent, they send events independently, we set the snowflake
// node id to 1 for one single agent, that means, we can sort events sent by the same agent.
Copy link
Member

@qiujian16 qiujian16 Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// node id to 1 for one single agent, that means, we can sort events sent by the same agent.
// init the snowflake id generator with node id 1 for each single agent.
// Each single agent has its own consumer id to be identified, and we
// can ensure the order of status update event from the same agent via
// sequence id. The events from different agents are independent, hence
// the ordering among them needs not to be guaranteed.

Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update the comment, others look good

/approve

Copy link
Contributor

openshift-ci bot commented Jan 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, skeeey

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Wei Liu <liuweixa@redhat.com>
Copy link
Member

@morvencao morvencao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 3, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 550ac68 into open-cluster-management-io:main Jan 3, 2024
15 checks passed
@skeeey skeeey deleted the sid branch January 17, 2024 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants