You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2021/04/14 07:53:43 UTC
[GitHub] [camel-k] squakez opened a new pull request #2217: feat(kamelets): kamelet binding error handler
squakez opened a new pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217
<!-- Description -->
With https://github.com/apache/camel-k-runtime/pull/648 we introduced the possibility to use a new `errorHandler` source type that can be used as default error handler builder for all routes derived from the same `Integration`. With this PR we enable a new `errorHandler` field in the `KameletBinding` spec which expect an error handler Kamelet.
The new Kamelet type can specify an error handler as with the traditional Camel business logic (ie examples/kamelets/error-handler/error-handler.kamelet.yaml): DLC, no-error, default.
That Kamelet will be treated as the other sources, but with the new `errorHandler` type that will be used to set the default error handler, if present.
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
NONE
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a change in pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on a change in pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#discussion_r621296486
##########
File path: deploy/olm-catalog/camel-k-dev/1.4.0/camel.apache.org_integrations.yaml
##########
@@ -88,6 +88,21 @@ spec:
items:
type: string
type: array
+ errorHandler:
Review comment:
That's correct, it sneaked in due to a change I did in the `Integration` in an interim commit. The autogen did not clean it up probably, so, removing that manually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] nicolaferraro merged pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
nicolaferraro merged pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] heiko-braun commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
heiko-braun commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-824558442
I assume ‚endpoint‘ is optional?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-824598041
Wondering if that is the right UX, i.e. not very easy to be described by a JSON Schema because there's no correlation between `type` and the other elements.
As example, you could write something like:
```yaml
errorHandler:
type: no
endpoint:
ref:
kind: Kamelet
apiVersion: camel.apache.org/v1alpha1
name: error-handler
properties:
message: "ERROR!"
configuration:
maximumRedeliveries: 3
redeliveryDelay: 2000
```
Which would be quire misleading so I wonder if instead of having the `type` as discriminator we should have a top level element, like:
```yaml
errorHandler:
dead-letter:
endpoint:
...
parameters:
...
```
Valid values for the top level field could be:
- dead-letter
- log (aka default but - IMHO - it is better being explicit)
- none
- builder|ref (this is to let the user pick any custom error handler provided by an `ErrorHandlerBuilder` bena)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-824621733
Thanks @lburgazzoli, I agree with your proposal as it feels more expressive. About the `builder|ref`, I wonder what we should expect here: just a reference to `bean` in the registry? Should we do something with that `bean` or we assume it must be there? also, wondering if it makes sense to name it as `bean` instead. From what I understand, the user that is planning to use it, have knowledge about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a change in pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on a change in pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#discussion_r613024183
##########
File path: pkg/controller/kameletbinding/common.go
##########
@@ -129,21 +145,38 @@ func createIntegrationFor(ctx context.Context, c client.Client, kameletbinding *
"to": to.URI,
})
- flow := map[string]interface{}{
+ flowFrom := map[string]interface{}{
"from": map[string]interface{}{
"uri": from.URI,
"steps": dslSteps,
},
}
- encodedFlow, err := json.Marshal(flow)
+ encodedFrom, err := json.Marshal(flowFrom)
if err != nil {
return nil, err
}
- it.Spec.Flows = append(it.Spec.Flows, v1.Flow{RawMessage: encodedFlow})
+ it.Spec.Flows = append(it.Spec.Flows, v1.Flow{RawMessage: encodedFrom})
return &it, nil
}
+func setErrorHandlerKamelet(errorHandler *bindings.Binding, kameletSpec v1alpha1.Endpoint) error {
Review comment:
I am not sure which would be the best approach for this part. I am using the `application.properties` as a place to store the error handler that will be eventually taken by the `kamelet` trait to be processed as a kamelet. Should we introduce an ad-hoc field on the `Integration` instead? I tried to put in the source `Meta` as we do for the `from` and `to` Kamelets, but those one are processed during the parse of the embedded route generated by the KameletBinding. @nicolaferraro @lburgazzoli any suggestion here is appreciated!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] lburgazzoli commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
lburgazzoli commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-824632419
about the `bean|ref` I do not yet have a clear vision, was wondering about having two options:
1. reference to a bean in the registry
```yaml
- ref: "ref-to-a-bean"
```
2. description of a bean to be created
```yaml
- bean:
type: "the-full-qualified-class-name"
properties:
foo: bar
```
This last option is a façade over the ref, where the operator should leverage the camel-main to [specify custom beans](https://camel.apache.org/components/3.7.x/others/main.html#_specifying_custom_beans) to register a bean to the registry. The generated properties could looks like:
```
camel.beans.global-error-handler = #class:the-full-qualified-class-name
camel.beans.global-error-handler.foo = bar
```
For consistency, the same mechanic can be used to implement the other error handler types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] nicolaferraro commented on a change in pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
nicolaferraro commented on a change in pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#discussion_r614698725
##########
File path: pkg/controller/kameletbinding/common.go
##########
@@ -129,21 +145,38 @@ func createIntegrationFor(ctx context.Context, c client.Client, kameletbinding *
"to": to.URI,
})
- flow := map[string]interface{}{
+ flowFrom := map[string]interface{}{
"from": map[string]interface{}{
"uri": from.URI,
"steps": dslSteps,
},
}
- encodedFlow, err := json.Marshal(flow)
+ encodedFrom, err := json.Marshal(flowFrom)
if err != nil {
return nil, err
}
- it.Spec.Flows = append(it.Spec.Flows, v1.Flow{RawMessage: encodedFlow})
+ it.Spec.Flows = append(it.Spec.Flows, v1.Flow{RawMessage: encodedFrom})
return &it, nil
}
+func setErrorHandlerKamelet(errorHandler *bindings.Binding, kameletSpec v1alpha1.Endpoint) error {
Review comment:
Ok, I think I got the way you're implementing this and I had a slightly different idea in mind. The runtime part should still be ok.
In your model, the error hander is a special type of Kamelet, while I was thinking we don't need another type of Kamelet, we can potentially set any Kamelet of type sink as error handler. In the model I'm describing, the error handler is a source that the operator adds to the list of "generated sources" in the integration status, possibly pointing to a standard Kamelet from the catalog.
This allows for example setting this binding:
```yaml
# ...
spec:
source:
# ...
sink:
# ...
errorHandler:
ref:
type: Kamelet
apiVersion: camel.apache.org/v1alpha1
name: dropbox-sink
```
From a user point of view, it means "store errors as files on dropbox", which seems natural.
But also, this allows setting something like:
```yaml
spec:
# ...
errorHandler:
ref:
type: Channel
apiVersion: messaging.knative.dev/v1
name: dead-letter-channel
```
I.e. "redirect errors to a Knative channel as-they-are".
This may work the following way, more or less:
1. The kameletbinding controller detects we want to use an error handler
2. The pkg/util/binding package contains methods for converting the `ref` + `properties` of the `errorHandler` definition into a Camel URI
3. The kameletbinding controller adds a generated source of type error-handler that forwards data in the URI determined at previous step
4. The pkg/metadata package should be instructed to detect URIs also in error handler definitions: this way the Kamelet trait will load the Kamelet as a standard sink-type kamelet (if you're using it, or do nothing if you're using e.g. a Knative channel as dead letter channel) and the error handler will point to it
Wdyt?
cc: @lburgazzoli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-827693102
> Just some minor issues and docs to be regenerated, but it overall looks very good!
Thanks a lot, just polished with the comments you provided!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a change in pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on a change in pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#discussion_r613024183
##########
File path: pkg/controller/kameletbinding/common.go
##########
@@ -129,21 +145,38 @@ func createIntegrationFor(ctx context.Context, c client.Client, kameletbinding *
"to": to.URI,
})
- flow := map[string]interface{}{
+ flowFrom := map[string]interface{}{
"from": map[string]interface{}{
"uri": from.URI,
"steps": dslSteps,
},
}
- encodedFlow, err := json.Marshal(flow)
+ encodedFrom, err := json.Marshal(flowFrom)
if err != nil {
return nil, err
}
- it.Spec.Flows = append(it.Spec.Flows, v1.Flow{RawMessage: encodedFlow})
+ it.Spec.Flows = append(it.Spec.Flows, v1.Flow{RawMessage: encodedFrom})
return &it, nil
}
+func setErrorHandlerKamelet(errorHandler *bindings.Binding, kameletSpec v1alpha1.Endpoint) error {
Review comment:
I am not sure which would be the best approach for this part. I am using the `application.properties` as a place to store the error handler that will be eventually taken by the `kamelet` trait to be processed as a kamelet. Should we introduce an ad-hoc field on the `Integration` instead? I tried to put in the source `Meta` as we do for the `from` and `to` Kamelets, but those one are processed during the parse of the embedded route generated by the KameletBinding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-827586717
@lburgazzoli @nicolaferraro I think this could be now ready for another review. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] nicolaferraro commented on a change in pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
nicolaferraro commented on a change in pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#discussion_r621232396
##########
File path: docs/modules/traits/pages/error-handler.adoc
##########
@@ -0,0 +1,36 @@
+= Error Handler Trait
+
+// Start of autogenerated code - DO NOT EDIT! (description)
+The error-handler is a platform trait used to inject Error Handler source into the integration runtime.
+
+
+This trait is available in the following profiles: **Kubernetes, Knative, OpenShift**.
+
+WARNING: The error-handler trait is a *platform trait*: disabling it may compromise the platform functionality.
+
+// End of autogenerated code - DO NOT EDIT! (description)
+// Start of autogenerated code - DO NOT EDIT! (configuration)
+== Configuration
+
+Trait properties can be specified when running any integration with the CLI:
+[source,console]
+----
+$ kamel run --trait error-handler.[key]=[value] --trait error-handler.[key2]=[value2] integration.groovy
+----
+The following configuration options are available:
+
+[cols="2m,1m,5a"]
+|===
+|Property | Type | Description
+
+| error-handler.enabled
+| bool
+| Can be used to enable or disable a trait. All traits share this common property.
+
+| error-handler.,omitempty
Review comment:
Seems there's a problem here
##########
File path: examples/kamelets/error-handler/error-handler.kamelet.yaml
##########
@@ -0,0 +1,41 @@
+# ---------------------------------------------------------------------------
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ---------------------------------------------------------------------------
+
+apiVersion: camel.apache.org/v1alpha1
+kind: Kamelet
+metadata:
+ name: error-handler
+spec:
+ definition:
+ title: "Error Log Sink"
+ description: "Consume events from a channel"
Review comment:
Wrong desc
##########
File path: deploy/olm-catalog/camel-k-dev/1.4.0/camel.apache.org_integrations.yaml
##########
@@ -88,6 +88,21 @@ spec:
items:
type: string
type: array
+ errorHandler:
Review comment:
After reading the PR, this seems something from previous attempts...
##########
File path: examples/kamelets/error-handler/incremental-id-source.kamelet.yaml
##########
@@ -0,0 +1,51 @@
+# ---------------------------------------------------------------------------
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ---------------------------------------------------------------------------
+
+apiVersion: camel.apache.org/v1alpha1
+kind: Kamelet
+metadata:
+ name: incremental-id-source
+ labels:
+ camel.apache.org/kamelet.type: "source"
+spec:
+ definition:
+ title: "Incremental ID Source"
+ description: "Produces periodic events with an incremental ID"
Review comment:
.. and fails when the ID contains "0"
##########
File path: deploy/olm-catalog/camel-k-dev/1.4.0/camel.apache.org_integrations.yaml
##########
@@ -88,6 +88,21 @@ spec:
items:
type: string
type: array
+ errorHandler:
Review comment:
Does it needs to be a top level property on the integration? I see there's a source of type "errorHandler" that can be added.
Also, you're updating the OLM manifest for 1.4.0 which is already released (I plan to only keep latest from now on btw, so it's not a real issue)
##########
File path: examples/kamelets/error-handler/error-handler.kamelet.yaml
##########
@@ -0,0 +1,41 @@
+# ---------------------------------------------------------------------------
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+# ---------------------------------------------------------------------------
+
+apiVersion: camel.apache.org/v1alpha1
+kind: Kamelet
+metadata:
+ name: error-handler
+spec:
+ definition:
+ title: "Error Log Sink"
+ description: "Consume events from a channel"
+ required:
+ - message
+ properties:
+ message:
+ title: Message
+ description: The message to log
+ type: string
+ example: "error while checking the source"
+ flow:
+ from:
+ uri: kamelet:source
+ steps:
+# - to: my-dlc
Review comment:
```suggestion
```
##########
File path: pkg/apis/camel/v1/integration_types_support.go
##########
@@ -131,6 +131,19 @@ func (in *IntegrationSpec) AddDependency(dependency string) {
in.Dependencies = append(in.Dependencies, newDep)
}
+// GetConfigurationProperty returns a configuration property
+func (in *IntegrationSpec) GetConfigurationProperty(property string) string {
+ for _, confSpec := range in.Configuration {
+ if confSpec.Type == "property" && strings.HasPrefix(confSpec.Value, property) {
+ splitConf := strings.Split(confSpec.Value, "=")
+ if len(splitConf) > 0 {
Review comment:
```suggestion
if len(splitConf) > 1 {
```
(don't know if "=" at the end of property returns 1 or 2 elements, but this does not panic)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-823319894
With 7ec6d0eba55f968686f66758f6e025cf215af73b I am introducing a more complex spec for an error handler (adding `type` and `configuration`). If the `type` is `dead-letter-channel`, then, an `endpoint` `ref` or `uri` can be used to deliver those messages (including another `kamelet`). This will cover the use case of a user willing to use a DLC with another kamelet.
Then, I've introduced a new trait, whose goal is to detect the presence of an error handler and create an error handler source type with the configuration provided by the user (according to the `type` right now, but I plan to include more `configuration`). This source will be used by the runtime as a default error handler as specified by https://github.com/apache/camel-k-runtime/pull/648
Please, consider it's still WIP, but I'll be happy to receive comments. cc @nicolaferraro @lburgazzoli
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #2217: feat(kamelets): kamelet binding error handler
Posted by GitBox <gi...@apache.org>.
squakez commented on pull request #2217:
URL: https://github.com/apache/camel-k/pull/2217#issuecomment-824591783
> I assume ‚endpoint‘ is optional?
That's correct. `endpoint` makes sense only when a `dead-letter-channel` is chosen. Probably we could add some check to make sure the combinations expected are valid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org