-
Notifications
You must be signed in to change notification settings - Fork 630
refactor in-memory channel to support sync and async handlers #708
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
Changes from all commits
031c09f
6666e3e
7a8b7fc
3894373
a286f38
370aa48
079c109
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,8 @@ import ( | |
| ) | ||
|
|
||
| const ( | ||
| ccpName = "in-memory-channel" | ||
| ccpName = "in-memory-channel" | ||
| asyncCCPName = "in-memory" | ||
|
|
||
| cNamespace = "test-namespace" | ||
| cName = "test-channel" | ||
|
|
@@ -518,6 +519,38 @@ func TestReconcile(t *testing.T) { | |
| events[channelReconciled], | ||
| }, | ||
| }, | ||
| { | ||
| Name: "Channel reconcile successful - Async channel", | ||
| // VirtualService should have channel provisioner name | ||
| // defaults to in-memory-channel but the service should match provisioner's service name | ||
| InitialState: []runtime.Object{ | ||
| makeChannel("in-memory"), | ||
| }, | ||
| Mocks: controllertesting.Mocks{}, | ||
| WantPresent: []runtime.Object{ | ||
| makeVirtualService(), | ||
| makeK8sService("in-memory"), | ||
| }, | ||
| WantEvent: []corev1.Event{ | ||
| events[channelReconciled], | ||
| }, | ||
| }, | ||
| { | ||
| Name: "Channel reconcile successful - Non Async channel", | ||
| // VirtualService should have channel provisioner name | ||
| // defaults to in-memory-channel | ||
| InitialState: []runtime.Object{ | ||
| makeChannel(), | ||
| }, | ||
| Mocks: controllertesting.Mocks{}, | ||
| WantPresent: []runtime.Object{ | ||
| makeVirtualService(), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I think this should have been |
||
| makeK8sService(), | ||
| }, | ||
| WantEvent: []corev1.Event{ | ||
| events[channelReconciled], | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
|
|
@@ -541,7 +574,7 @@ func TestReconcile(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func makeChannel() *eventingv1alpha1.Channel { | ||
| func makeChannel(pn ...string) *eventingv1alpha1.Channel { | ||
| c := &eventingv1alpha1.Channel{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: eventingv1alpha1.SchemeGroupVersion.String(), | ||
|
|
@@ -554,14 +587,24 @@ func makeChannel() *eventingv1alpha1.Channel { | |
| }, | ||
| Spec: eventingv1alpha1.ChannelSpec{ | ||
| Provisioner: &corev1.ObjectReference{ | ||
| Name: ccpName, | ||
| Name: getProvisionerName(pn), | ||
| }, | ||
| }, | ||
| } | ||
| c.Status.InitializeConditions() | ||
| return c | ||
| } | ||
|
|
||
| // getProvisionerName returns either default provisioner name defined by ccpName variable | ||
| // or, if specified, a custom provisioner name. | ||
| func getProvisionerName(pn []string) string { | ||
| provisionerName := ccpName | ||
| if len(pn) != 0 { | ||
| provisionerName = pn[0] | ||
| } | ||
| return provisionerName | ||
| } | ||
|
|
||
| func makeChannelWithFinalizerAndAddress() *eventingv1alpha1.Channel { | ||
| c := makeChannelWithFinalizer() | ||
| c.Status.SetAddress(serviceAddress) | ||
|
|
@@ -631,7 +674,7 @@ func makeConfigMapWithVerifyConfigMapData() *corev1.ConfigMap { | |
| return cm | ||
| } | ||
|
|
||
| func makeK8sService() *corev1.Service { | ||
| func makeK8sService(pn ...string) *corev1.Service { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion for a different PR: Seems like this could benefit from the builder pattern for making fixtures with varying aspects. That might make it more clear to the reader what these arguments mean. See builder.go and SubscriptionBuilder.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for pointing out. |
||
| return &corev1.Service{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: "v1", | ||
|
|
@@ -643,8 +686,8 @@ func makeK8sService() *corev1.Service { | |
| Labels: map[string]string{ | ||
| util.EventingChannelLabel: cName, | ||
| util.OldEventingChannelLabel: cName, | ||
| util.EventingProvisionerLabel: ccpName, | ||
| util.OldEventingProvisionerLabel: ccpName, | ||
| util.EventingProvisionerLabel: getProvisionerName(pn), | ||
| util.OldEventingProvisionerLabel: getProvisionerName(pn), | ||
| }, | ||
| OwnerReferences: []metav1.OwnerReference{ | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,9 @@ const ( | |
| // Configuration for a fanout.Handler. | ||
| type Config struct { | ||
| Subscriptions []eventingduck.ChannelSubscriberSpec `json:"subscriptions"` | ||
| // AsyncHandler controls whether the Subscriptions are called synchronous or asynchronously. | ||
| // It is expected to be false when used as a sidecar. | ||
| AsyncHandler bool `json:"asyncHandler,omitempty"` | ||
| } | ||
|
|
||
| // http.Handler that takes a single request in and fans it out to N other servers. | ||
|
|
@@ -83,6 +86,13 @@ func NewHandler(logger *zap.Logger, config Config) *Handler { | |
|
|
||
| func createReceiverFunction(f *Handler) func(provisioners.ChannelReference, *provisioners.Message) error { | ||
| return func(_ provisioners.ChannelReference, m *provisioners.Message) error { | ||
| if f.config.AsyncHandler { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unit test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Modified existing unit test by adding one test case where AsyncHandler is set true. Let me know if you think it is sufficient.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think anything has been pushed. |
||
| go func() { | ||
| // Any returned error is already logged in f.dispatch(). | ||
| _ = f.dispatch(m) | ||
| }() | ||
| return nil | ||
| } | ||
| return f.dispatch(m) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,24 @@ $(dirname $0)/upload-test-images.sh e2e || fail_test "Error uploading test image | |
| # Setup resources common to all eventing tests | ||
| setup_events_test_resources|| fail_test "Error setting up test resources" | ||
|
|
||
| go_test_e2e ./test/e2e || fail_test | ||
| go_test_e2e ./test/e2e | ||
| exit_result=$? | ||
| if [ ${exit_result} -ne 0 ]; then | ||
| # Collecting logs from all knative's eventing pods | ||
| echo "============================================================" | ||
| for namespace in "knative-eventing" "e2etestfn3"; do | ||
| for pod in $(kubectl get pod -n $namespace | grep Running | awk '{print $1}' ); do | ||
| for container in $(kubectl get pod "${pod}" -n $namespace -ojsonpath='{.spec.containers[*].name}'); do | ||
| echo "Namespace, Pod, Container: ${namespace}, ${pod}, ${container}" | ||
| kubectl logs -n $namespace "${pod}" -c "${container}" || true | ||
| echo "----------------------------------------------------------" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also write out that this is a previous incarnation of a Pod. Something like: echo "Namespace, Pod, Container -- Previous Instance: ${namespace}, ${pod}, ${container}"
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| echo "Namespace, Pod, Container (Previous instance): ${namespace}, ${pod}, ${container}" | ||
| kubectl logs -p -n $namespace "${pod}" -c "${container}" || true | ||
| echo "============================================================" | ||
| done | ||
| done | ||
| done | ||
| fail_test | ||
| fi | ||
|
|
||
| success | ||
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
makeK8sService("in-memory")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.
done