-
Notifications
You must be signed in to change notification settings - Fork 21
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
multiple instances support for grpc broker. #235
base: main
Are you sure you want to change the base?
Conversation
/ok-to-test |
/hold depends on #232 |
1ce55ab
to
580601f
Compare
pkg/controllers/event_handler.go
Outdated
// check if all instances have processed the event | ||
if !compareStrings(activeInstances, eventInstances) { | ||
klog.V(10).Infof("Event %s is not processed by all instances, handled by %v, active instances %v", event.ID, eventInstances, activeInstances) | ||
return nil |
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.
do we need requeue here (return an error)? or there are some processes can retrigger this?
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.
In this case, reconcileDate is not set, the 'SyncEvent' will trigger event be processed again?
If we return an error here, the current instance will process the event again, but the event may not be handled by other instances. so return an error doesn't help.
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'm thinking maybeactiveInstances
is_subset_of eventInstances
make more sense?
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.
If we return an error here, the current instance will process the event again, but the event may not be handled by other instances. so return an error doesn't help.
so this means the ReconciledDate
will be marked finally by the the "last" instance, right?
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, I added some comments to explain different cases.
if svcErr != nil { | ||
// if the resource is not found, it indicates the resource has been handled by | ||
// other instances, so we can mark the event as reconciled and ignore it. | ||
if svcErr.Is404() { |
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.
add a log here
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.
log added.
580601f
to
29049f9
Compare
Signed-off-by: morvencao <[email protected]>
1c1601c
to
1e1e107
Compare
Signed-off-by: morvencao <[email protected]>
281ccd4
to
acaa814
Compare
Signed-off-by: morvencao <[email protected]>
acaa814
to
5f9e8f3
Compare
} | ||
|
||
klog.V(4).Infof("send the event to spec subscribers, %s", evt) | ||
|
||
// WARNING: don't use "pbEvt, err := pb.ToProto(evt)" to convert cloudevent to protobuf | ||
pbEvt := &pbv1.CloudEvent{} | ||
if err = grpcprotocol.WritePBMessage(context.TODO(), binding.ToMessage(evt), pbEvt); err != nil { | ||
return fmt.Errorf("failed to convert cloudevent to protobuf: %v", err) | ||
klog.Errorf("failed to convert cloudevent to protobuf: %v", err) |
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.
also record the resource ID
ref: https://issues.redhat.com/browse/ACM-16035
/assign @skeeey @qiujian16 @clyang82