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

Configurable subscription interval time #2798

Conversation

patricgruber
Copy link
Contributor

This pull requests makes the subscription refresh time configurable via environment variable as described in #1928

@patricgruber
Copy link
Contributor Author

I'm not totally sure if this is the best way to do it, so I'd be happy to get feedback on this!
The documentation for the scheduler changes: https://docs.nestjs.com/techniques/task-scheduling#dynamic-intervals

@patricgruber
Copy link
Contributor Author

The corresponding change in the documentation: ViewTube/wiki#10

@moisout
Copy link
Member

moisout commented Jun 2, 2024

Thank you very much for this PR! Very sorry it took me so long, I'll take a look at the changes ASAP

@moisout
Copy link
Member

moisout commented Jun 3, 2024

@patricgruber Looks very good already

private schedulerRegistry: SchedulerRegistry
) {
// initialize without blocking
void this.initializeSubscriptionTask();
Copy link
Member

Choose a reason for hiding this comment

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

I would run this in the main.ts file, like this.

const subscriptionsService = app.get(SubscriptionsService);
void subscriptionsService.initializeSubscriptionTask();

videos: [],
videoCount: 0,
lastRefresh: null,
refreshInterval: this.getSubscriptionIntervalTime()
Copy link
Member

Choose a reason for hiding this comment

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

Please add this property to the SubscriptionFeedResponseDto (subscription-feed-response.dto.ts)

@moisout
Copy link
Member

moisout commented Jun 3, 2024

When testing, I am getting this error when the job is running, do you get this too?
image

@patricgruber
Copy link
Contributor Author

Thank you for the quick review! I changed the code as requested

@patricgruber
Copy link
Contributor Author

When testing, I am getting this error when the job is running, do you get this too?

For me everything seems to be fine:
image

But I'm using pnpm run build && pnpm run start from within the server directory because pnpm run:serve doesn't work for me. Maybe this causes a discrepancy between our set-ups.

@moisout
Copy link
Member

moisout commented Jun 4, 2024

But I'm using pnpm run build && pnpm run start from within the server directory because pnpm run:serve doesn't work for me. Maybe this causes a discrepancy between our set-ups.

I see, there may be a difference between the two

@moisout
Copy link
Member

moisout commented Jun 6, 2024

I think this looks perfect now, do agree?

@patricgruber
Copy link
Contributor Author

Yes, looks good to me!

@moisout moisout merged commit b135c30 into ViewTube:development Jun 6, 2024
5 checks passed
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