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-42 consumer priority groups #279

Merged
merged 1 commit into from
Jul 18, 2024
Merged

ADR-42 consumer priority groups #279

merged 1 commit into from
Jul 18, 2024

Conversation

ripienaar
Copy link
Contributor

No description provided.

@ripienaar
Copy link
Contributor Author

This is a draft, I have some work left to do here and some thinking but enough to get some feedback, i am not feeling too hot today but wanted to get this out, please discuss here.

adr/ADR-42.md Outdated 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.

Some comments and thoughts around details and future proofing.

adr/ADR-42.md Outdated Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved
@ripienaar
Copy link
Contributor Author

cc: @bruth lots of hope to get this in 2.11

adr/ADR-42.md Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved
adr/ADR-42.md Outdated Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved
```go
type PriorityGroupState struct {
Group string `json:"name"`
PinnedClientId string `json:"pinned_id,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

SelectedClientId
LeaseTs

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 - I put some suggestions on terms/option names

Copy link
Contributor

@jnmoyne jnmoyne left a comment

Choose a reason for hiding this comment

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

Just some minor test edit suggestions, and some comments (things to be captured for when the documentation is going to be written based on this document). LGTM.

adr/ADR-42.md Outdated Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved
adr/ADR-42.md Outdated Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved

* We have 1 group defined and all pulls have to belong to this group
* The policy is `pinned_client` that activates these behaviors
* When a pinned client has not done any pulls in the last 120 seconds the server will switch to another client
Copy link
Member

Choose a reason for hiding this comment

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

With this approach, longer expire interval, more sensitive the TTL becomes.
Maybe we should base TTL on Duration since last Pinned Client Pull Request expiry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be an option, my thinking was that people will tune this based on their usage, network, maintenance behaviours etc rather than have a lot of magic.

We could move to magic in a future version though but I am reluctant to start with magic.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW in my initial consumer group implementation I am using the idle timeout for the consumer (i.e. time since last pull request expired) and it's working for me. Otherwise what if you set the timeout to for example 60s and the applications are fetching with a timeout of more than 60s, wouldn't that lead to flapping when there are no new messages to deliver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not clear here but like the way it calculates if a consumer is idle or not it wouldn’t consider a client with an flight pull as being active.

But this seems open to bugs or abuse so perhaps you simply would have to ensure you pull more frequently. Alternatively we could cap the pull timeout so it relates to this timeout - no more than twice this timeout pull expire or something?

good point. Needs some thinking here

adr/ADR-42.md Outdated Show resolved Hide resolved
adr/ADR-42.md Show resolved Hide resolved
adr/ADR-42.md Outdated Show resolved Hide resolved
@ripienaar ripienaar marked this pull request as ready for review July 15, 2024 08:15
This was referenced Jul 17, 2024
@ripienaar
Copy link
Contributor Author

@Jarema see 7250d66 to summarise our call about config updates

@ripienaar ripienaar changed the title wip adr for consumer priority groups ADR-42 consumer priority groups Jul 17, 2024
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

@ripienaar ripienaar merged commit 09054a4 into main Jul 18, 2024
1 check passed
@ripienaar ripienaar deleted the priority_group branch July 18, 2024 09:25
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.

4 participants