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.

Sign up for GitHub

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

Jump to bottom

Don't use async handler #7415

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Don't use async handler #7415

merged 4 commits into from
Nov 14, 2023

Conversation

Cali0707
Copy link
Member

To try to improve the consistency of the system when using the IMC, this PR is testing switching to using the sync handler

Proposed Changes

  • Use the sync handler instead of the async handler

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

The InMemoryChannel now sends a 202 response only after successfully delivering the event to all subscribers

Docs

@Cali0707
Don't use async handler
8dd3e6b 
Signed-off-by: Calum Murray <[email protected]>
@knative-prow knative-prow bot requested review from aliok and odacremolbap October 27, 2023 14:32
@knative-prow knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 27, 2023
@Cali0707
Copy link
Member Author

/cc @pierDipi
/hold
For testing a few times :)

@knative-prow knative-prow bot requested a review from pierDipi October 27, 2023 14:32
@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2023
@codecov
Copy link

codecov bot commented Oct 27, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0684dbe) 76.74% compared to head (0e701d3) 76.74%.
Report is 13 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #7415   +/-   ##
=======================================
  Coverage   76.74%   76.74%           
=======================================
  Files         253      253           
  Lines       13916    13916           
=======================================
  Hits        10680    10680           
  Misses       2702     2702           
  Partials      534      534

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Cali0707
Copy link
Member Author

/retest

Cali0707
Cali0707 commented Oct 30, 2023
View reviewed changes
pkg/channel/fanout/fanout_event_handler.go
@@ -302,7 +303,6 @@ func (f *FanoutEventHandler) dispatch(ctx context.Context, subs []Subscription,
if dispatchResult.err != nil {
f.logger.Error("Fanout had an error", zap.Error(dispatchResult.err))
dispatchResultForFanout.err = dispatchResult.err
return dispatchResultForFanout
Copy link
Member Author

Choose a reason for hiding this comment

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

@pierDipi I think this early return may cause a race condition in the synchronous case, as it may lead to the context being cancelled (request completes) before all of the dead letter sink requests have been made if there is more than one error.

However, this means that we won't aggregate the errors and are only showing the final error. Should we add some kind of error aggregation here?

Copy link
Member

Choose a reason for hiding this comment

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

Error aggregation sounds good, we could track it into a separate issue

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2023
@Cali0707 Cali0707 force-pushed the use-sync-handler-imc branch from d02844b to ff97c1d Compare October 30, 2023 16:13
@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2023
@Cali0707
try refactoring test
54078cd 
Signed-off-by: Calum Murray <[email protected]>
@Cali0707 Cali0707 force-pushed the use-sync-handler-imc branch from cf33d77 to 54078cd Compare October 30, 2023 18:50
@Cali0707
take 2 on test fix
0e701d3 
Signed-off-by: Calum Murray <[email protected]>
@knative-prow knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2023
Cali0707
Cali0707 commented Oct 30, 2023
View reviewed changes
test/rekt/broker_test.go
env.TestSet(ctx, t, broker.DataPlaneConformance("default"))
env.TestSet(ctx, t, broker.ControlPlaneConformance("default", b.WithEnvConfig()...))
Copy link
Member Author

Choose a reason for hiding this comment

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

@pierDipi changing the order of these tests caused the DataPlaneConformance tests to pass 😕

@Cali0707
Copy link
Member Author

/retest

@Cali0707
Copy link
Member Author

/test upgrade-tests
(this is what we are trying to improve)

@Cali0707
Copy link
Member Author

Cali0707 commented Nov 1, 2023

/test upgrade-tests
(retesting again to see if the flakes are gone)

@pierDipi
Copy link
Member

3/3 green on the last commit (retesting again to see if the flakes are gone)
/test upgrade-tests

@Cali0707
Copy link
Member Author

Still green, going to try again for the flakes:
/test upgrade-tests

@Cali0707
Copy link
Member Author

/test upgrade-tests

@Cali0707
Copy link
Member Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2023
pierDipi
pierDipi approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Great!

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2023
@knative-prow Knative Prow
Copy link

knative-prow bot commented Nov 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, pierDipi

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

@knative-prow knative-prow bot merged commit d964da3 into knative:main Nov 14, 2023
1 check passed
@Cali0707 Cali0707 mentioned this pull request Nov 15, 2023
Closed
This was referenced Dec 7, 2023
Merged
Merged
Cali0707 added a commit to Cali0707/eventing that referenced this pull request Feb 1, 2024
@Cali0707
Don't use async handler (knative#7415)
58d26b7 
* Don't use async handler

Signed-off-by: Calum Murray <[email protected]>

* Fix Kn-Namespace header, potential race condition for sync receiver

Signed-off-by: Calum Murray <[email protected]>

* try refactoring test

Signed-off-by: Calum Murray <[email protected]>

* take 2 on test fix

Signed-off-by: Calum Murray <[email protected]>

---------

Signed-off-by: Calum Murray <[email protected]>
Cali0707 added a commit to Cali0707/eventing that referenced this pull request Feb 1, 2024
@Cali0707
Don't use async handler (knative#7415)
54644a8 
* Don't use async handler

Signed-off-by: Calum Murray <[email protected]>

* Fix Kn-Namespace header, potential race condition for sync receiver

Signed-off-by: Calum Murray <[email protected]>

* try refactoring test

Signed-off-by: Calum Murray <[email protected]>

* take 2 on test fix

Signed-off-by: Calum Murray <[email protected]>

---------

Signed-off-by: Calum Murray <[email protected]>
openshift-merge-bot bot pushed a commit to openshift-knative/eventing that referenced this pull request Feb 2, 2024
@Cali0707
Don't use async handler (knative#7415) (#515)
2303bba 
* Don't use async handler



* Fix Kn-Namespace header, potential race condition for sync receiver



* try refactoring test



* take 2 on test fix



---------

Signed-off-by: Calum Murray <[email protected]>
openshift-merge-bot bot pushed a commit to openshift-knative/eventing that referenced this pull request Feb 2, 2024
@Cali0707
Don't use async handler (knative#7415) (#516)
6a034ac 
* Don't use async handler



* Fix Kn-Namespace header, potential race condition for sync receiver



* try refactoring test



* take 2 on test fix



---------

Signed-off-by: Calum Murray <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi approved these changes

@aliok aliok Awaiting requested review from aliok

@odacremolbap odacremolbap Awaiting requested review from odacremolbap

Assignees

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Cali0707 @pierDipi