-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Ttl configuration merge #3210
Ttl configuration merge #3210
Conversation
… nv-1931-execution-details-jobs
…-large' into nv-1931-execution-details-jobs # Conflicts: # apps/api/src/app/events/usecases/create-notification-jobs/create-notification-jobs.usecase.ts # libs/dal/src/repositories/execution-details/execution-details.schema.ts
…ails-jobs # Conflicts: # apps/api/src/app/events/usecases/add-job/add-delay-job.usecase.ts # libs/dal/src/repositories/base-repository.ts # libs/dal/src/repositories/notification/notification.schema.ts
…into ttl-configuration-merge # Conflicts: # apps/api/src/app/events/e2e/delay-events.e2e.ts # apps/api/src/app/events/events.module.ts # packages/application-generic/src/usecases/add-job/add-delay-job.usecase.ts # packages/application-generic/src/usecases/add-job/add-job.usecase.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌟
@@ -0,0 +1 @@ | |||
export const TTL_EXPIRE_AFTER_AMOUNT = '48h'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only thing that makes me wonder a bit.
I feel like this value is business decision that is being configured rather than a database layer configuration, so it makes me think it should leave elsewhere and be able to be configurable via environment variables, allowing us to test the feature it configures and set different values for different environments.
We can discuss it further in a following PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific one is only a buffer, I believe what you are referring to is the months addition here . And I agree :)
In the future I believe it will be in the database - depending on how we decide to handle user tiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using it in the schema index creation thus my wondering.
…ion-for-saas-and add ttl index configuration
# Conflicts: # apps/api/src/app/events/events.module.ts # apps/worker/src/app/workflow/workflow.module.ts # packages/application-generic/src/usecases/create-notification-jobs/create-notification-jobs.usecase.ts
What change does this PR introduce?
Replaces this PR .
Why was this change needed?
Other information (Screenshots)