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 more information about Broker Developer Configuration Options #5993

Merged
merged 6 commits into from
Jun 7, 2024

Conversation

pembebiri
Copy link
Contributor

@pembebiri pembebiri commented May 23, 2024

Copy link

linux-foundation-easycla bot commented May 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@knative-prow knative-prow bot requested review from Cali0707 and pierDipi May 23, 2024 22:15
Copy link

knative-prow bot commented May 23, 2024

Welcome @0ZeKa0! It looks like this is your first PR to knative/docs 🎉

@knative-prow knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 23, 2024
Copy link

netlify bot commented May 23, 2024

Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 98c62ea
🔍 Latest deploy log https://app.netlify.com/sites/knative/deploys/6662a93ed36bb20008a8f72f
😎 Deploy Preview https://deploy-preview-5993--knative.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pembebiri pembebiri closed this May 23, 2024
@pembebiri pembebiri deleted the broker-dev branch May 23, 2024 23:08
@pembebiri pembebiri restored the broker-dev branch May 23, 2024 23:08
@pembebiri pembebiri reopened this May 23, 2024
@pembebiri pembebiri closed this May 24, 2024
@pembebiri pembebiri reopened this May 24, 2024
Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

Hello @0ZeKa0,
thanks a lot for your work on this 👍
I left a few comments.
When I checked on your PR, I was thinking about merging the "Developer configuration options" and "Creating a Broker" page. Any objections on that? Otherwise we can continue with the split approach as in this PR to describe the needed options and then merge them later 🤷

@@ -1,6 +1,6 @@
# Developer configuration options

## Broker configuration example
## Broker configuration
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the full example to the end of the page. So we describe before the configuration options and give then a "full example"?

Copy link
Contributor Author

@pembebiri pembebiri May 29, 2024

Choose a reason for hiding this comment

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

Moving the full example would be better because a section from options was repeated on the Developer Configuration Options.


### Event Delivery Options
- You can use `dead-letter sink` for error handling and auditing of undelivered messages. Specify Kubernetes object reference where undelivered messages will be sent using `ref` and an optional URI to route undelivered messages using `uri`.
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponantial` or `linear`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponantial` or `linear`.
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponantial` or `linear`.

### Event Delivery Options
- You can use `dead-letter sink` for error handling and auditing of undelivered messages. Specify Kubernetes object reference where undelivered messages will be sent using `ref` and an optional URI to route undelivered messages using `uri`.
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponantial` or `linear`.
- You can set the `Backoff delay ` to specify the initial delay before retrying, using the ISO 8601 duration format.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- You can set the `Backoff delay ` to specify the initial delay before retrying, using the ISO 8601 duration format.
- You can set the `Backoff delay` to specify the initial delay before retrying, using the ISO 8601 duration format.

- `spec.delivery` is used to configure event delivery options. Event delivery options specify what happens to an event that fails to be delivered to an event sink. For more information, see the documentation on [Event delivery](../event-delivery.md).

### Advance broroker class options
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Advance broroker class options
### Advanced Broker class options

- `spec.delivery` is used to configure event delivery options. Event delivery options specify what happens to an event that fails to be delivered to an event sink. For more information, see the documentation on [Event delivery](../event-delivery.md).

### Advance broroker class options
When a Broker is created without a specified `eventing.knative.dev/broker.class` annotation, the default `MTChannelBasedBroker` Broker class is used, as specified by default in the `config-br-defaults` ConfigMap.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When a Broker is created without a specified `eventing.knative.dev/broker.class` annotation, the default `MTChannelBasedBroker` Broker class is used, as specified by default in the `config-br-defaults` ConfigMap.
When a Broker is created without a specified `eventing.knative.dev/broker.class` annotation, by default the `MTChannelBasedBroker` Broker class is used, as specified in the `config-br-defaults` ConfigMap.

@aliok
Copy link
Member

aliok commented May 28, 2024

Thanks @0ZeKa0 !


### Event Delivery Options
- You can use `dead-letter sink` for error handling and auditing of undelivered messages. Specify Kubernetes object reference where undelivered messages will be sent using `ref` and an optional URI to route undelivered messages using `uri`.
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponantial` or `linear`.
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponantial` or `linear`.
- You can set the `Backoff policies` to define the delay strategy between retry attempts. It can be `exponential` or `linear`.

@pembebiri
Copy link
Contributor Author

I thought that the expectation was merging just "Broker class options" section to "Developer Configuration Options" page. So let's continue with split approach. @creydr

 The full example moved to end of the page
Copy link
Contributor Author

@pembebiri pembebiri left a comment

Choose a reason for hiding this comment

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

reviewed

Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

one small nit. Looks LGTM otherwise

Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

Thanks a lot @0ZeKa0 for fixing this 💪

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2024
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

/approve

Copy link

knative-prow bot commented Jun 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0ZeKa0, Cali0707, creydr

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2024
@knative-prow knative-prow bot merged commit 946e2ae into knative:main Jun 7, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants