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 support for cascading retries #35

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

derrickburns
Copy link
Contributor

@todd I have not tested this yet. I just want to get your eyes on it to see if you a better approach.

select {
case <-timer.C:
break
case <-session.Context().Done():
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the context terminated when a rebalance happens?

events/config.go Outdated
@@ -19,6 +21,9 @@ type CloudEventsConfig struct {
KafkaVersion string `envconfig:"KAFKA_VERSION" required:"true"`
KafkaUsername string `envconfig:"KAFKA_USERNAME" required:"false"`
KafkaPassword string `envconfig:"KAFKA_PASSWORD" required:"false"`
KafkaDelay int `envconfig:"KAFKA_DELAY" default:"0"`
CascadeDelays []int `envconfig:"KAFKA_CASCADE_DELAYS" default:""`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think envocnfig supports parsing time.Duration out of the box, so it should be possible to use it directly, instead of int.

Should we provide sensible defaults?

Copy link
Contributor

@toddkazakov toddkazakov 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 - just make sure rebalancing is handled correctly

@derrickburns
Copy link
Contributor Author

I added support for exponential backoff retries.

toddkazakov
toddkazakov previously approved these changes Nov 7, 2020
Copy link
Contributor

@toddkazakov toddkazakov left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Can you please test this before merging?

@derrickburns
Copy link
Contributor Author

derrickburns commented Nov 7, 2020 via email

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.

2 participants