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

ADR-43: message ttl #292

Merged
merged 2 commits into from
Jul 16, 2024
Merged

ADR-43: message ttl #292

merged 2 commits into from
Jul 16, 2024

Conversation

ripienaar
Copy link
Contributor

No description provided.

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

Looks good! Some comments and thoughts added.

adr/ADR-43.md Outdated Show resolved Hide resolved
adr/ADR-43.md Outdated Show resolved Hide resolved
adr/ADR-43.md Outdated Show resolved Hide resolved
adr/ADR-43.md Show resolved Hide resolved
@ripienaar ripienaar marked this pull request as ready for review July 11, 2024 10:43
adr/ADR-43.md Outdated Show resolved Hide resolved
@ripienaar ripienaar changed the title wip: ADR-31 message ttl ADR-31: message ttl Jul 11, 2024
adr/ADR-43.md Outdated Show resolved Hide resolved
adr/ADR-43.md Show resolved Hide resolved
adr/ADR-43.md Outdated Show resolved Hide resolved
adr/ADR-43.md Show resolved Hide resolved
adr/ADR-43.md Outdated Show resolved Hide resolved
@ripienaar ripienaar changed the title ADR-31: message ttl ADR-43: message ttl Jul 11, 2024
Signed-off-by: R.I.Pienaar <[email protected]>
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

Just few last comments

adr/ADR-43.md Outdated Show resolved Hide resolved
adr/ADR-43.md Show resolved Hide resolved
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@aricart aricart self-requested a review July 12, 2024 15:40
Copy link
Member

@aricart aricart left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: R.I.Pienaar <[email protected]>
@ripienaar
Copy link
Contributor Author

Have changed the wording around MaxAge rather as a admin control for resource management its stated as the default when not set on a message and also the maximum allowed except for the never expire ones.

We can later restrict that with a config bool if we want but I dont want to add 3 new stream configs right at the start if it can be avoided or later end up not being needed

I'll merge this one end of day baring other feedback.

@ripienaar ripienaar merged commit e53b2c3 into main Jul 16, 2024
1 check passed
@ripienaar ripienaar deleted the msg_ttl branch July 16, 2024 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants