Skip to content

fix: configure discard policy for WorkQueue/Interest #1884

Merged
vigith merged 2 commits intonumaproj:mainfrom
kohlisid:isbpolicy
Jul 31, 2024
Merged

fix: configure discard policy for WorkQueue/Interest #1884
vigith merged 2 commits intonumaproj:mainfrom
kohlisid:isbpolicy

Conversation

@kohlisid
Copy link
Copy Markdown
Contributor

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.48%. Comparing base (e7c32c1) to head (e01f901).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1884      +/-   ##
==========================================
+ Coverage   54.31%   54.48%   +0.17%     
==========================================
  Files         288      288              
  Lines       28301    28306       +5     
==========================================
+ Hits        15371    15422      +51     
+ Misses      11994    11953      -41     
+ Partials      936      931       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vigith
Copy link
Copy Markdown
Member

vigith commented Jul 31, 2024

should we add a check so that nobody will turn on DiscardOld with WorkQueue or DiscardNew with Limits ?

@vigith vigith changed the title chore: allow configurable retention policy fix: allow configurable retention policy Jul 31, 2024
@kohlisid
Copy link
Copy Markdown
Contributor Author

In that case do you want to tie both the properties together instead of configurable?

  1. If Limits/Interest is given -> use discardOld
  2. If workqueue is given -> use discardNew

Comment thread pkg/isbsvc/jetstream_service.go Outdated
retention := nats.RetentionPolicy(v.GetInt("stream.retention"))
discard := nats.DiscardPolicy(v.GetInt("stream.discardPolicy"))
// we cannot use DiscardNew with Limits policy.
if retention == nats.LimitsPolicy && discard == nats.DiscardNew {
Copy link
Copy Markdown
Contributor

@Krithika3 Krithika3 Jul 31, 2024

Choose a reason for hiding this comment

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

Should we also check if it is not the other? WorkQueue with DiscardOld?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nevermind, just saw that @vigith has already made that comment.

@vigith
Copy link
Copy Markdown
Member

vigith commented Jul 31, 2024

In that case do you want to tie both the properties together instead of configurable?

  1. If Limits/Interest is given -> use discardOld
  2. If workqueue is given -> use discardNew

I would think so, @whynowy ?

Comment thread config/install.yaml Outdated
replicas: 3
duplicates: 60s
# 0: DiscardOld, 1: DiscardNew
discardPolicy: 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just curious, why do we have multiple configs where we have this field set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use the different installation strategies for the numaflow controller, so replicating it for all.

Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
@kohlisid kohlisid marked this pull request as ready for review July 31, 2024 22:40
@kohlisid kohlisid requested review from vigith and whynowy as code owners July 31, 2024 22:40
@vigith vigith linked an issue Jul 31, 2024 that may be closed by this pull request
Comment thread pkg/isbsvc/jetstream_service.go Outdated
Comment on lines +131 to +133
if retention == nats.WorkQueuePolicy {
discard = nats.DiscardNew
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what about Interest? Why should it be DiscardOld? please document

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I went ahead as it was default, but I realised it didn't seem to be correct.
We should use it with DiscardNew instead
I have added few lines to document, please take a look @vigith

Signed-off-by: Sidhant Kohli <sidhant.kohli@gmail.com>
@kohlisid kohlisid requested a review from vigith July 31, 2024 23:21
@kohlisid kohlisid changed the title fix: allow configurable retention policy fix: configure discard policy for WorkQueue/Interest Jul 31, 2024
@vigith vigith enabled auto-merge (squash) July 31, 2024 23:42
@vigith vigith merged commit 280b9bd into numaproj:main Jul 31, 2024
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.

Enable with WorkQueue policy

4 participants