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

feat: Integrate pg-boss for job scheduling #478

Closed

Conversation

Allan2000-Git
Copy link
Contributor

@Allan2000-Git Allan2000-Git commented Oct 5, 2024

User description

Description

This PR adds a integrates pg-boss for job scheduling. This allows us to run jobs in the background as cron jobs or jobs that might take up significant resources.

Fixes #138

Dependencies

Installed packages: pg-boss

Future Improvements

N/A

Mentions

@rajdip-b

Screenshots of relevant screens

N/A

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement


Description

  • Integrated pg-boss for job scheduling by adding a new JobHandler class.
  • Updated CommonModule to include JobHandler in providers and exports.
  • Added pg-boss to the project dependencies in package.json.
  • Updated pnpm-lock.yaml to reflect the new dependencies.

Changes walkthrough 📝

Relevant files
Enhancement
common.module.ts
Add JobHandler to CommonModule providers and exports         

apps/api/src/common/common.module.ts

  • Added JobHandler to the providers and exports arrays.
+3/-2     
job.handler.ts
Implement JobHandler class for job scheduling                       

apps/api/src/common/job.handler.ts

  • Created JobHandler class for managing jobs with pg-boss.
  • Implemented methods to initialize, register, schedule, and stop jobs.
  • Added error handling and logging for job operations.
  • +68/-0   
    Dependencies
    package.json
    Add pg-boss dependency for job scheduling                               

    apps/api/package.json

  • Added pg-boss as a new dependency.
  • Adjusted the order of reflect-metadata in devDependencies.
  • +2/-1     
    pnpm-lock.yaml
    Update pnpm-lock.yaml with pg-boss dependencies                   

    pnpm-lock.yaml

    • Updated lock file to include pg-boss and its dependencies.
    +269/-10

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The error handling in the initialize, registerJob, and scheduleJob methods could be improved. Currently, errors are logged and then rethrown, which may lead to unhandled promise rejections.

    Logging Strategy
    The class uses console.log and console.error for logging. Consider using a more robust logging solution that allows for different log levels and easier log management.

    Dependency Injection
    The JobHandler is added to the providers and exports arrays, but it's unclear how its dependencies (like the connection string) are being injected.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Oct 5, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    ✅ Implement a graceful shutdown mechanism for PgBoss

    Consider implementing a graceful shutdown mechanism. When the stop method is called,
    it should wait for any ongoing jobs to complete before stopping PgBoss. This ensures
    that no jobs are abruptly terminated, which could lead to data inconsistencies.

    apps/api/src/common/job.handler.ts [57-64]

    -async stop(): Promise<void> {
    +async stop(gracePeriod: number = 5000): Promise<void> {
       try {
    -    await this.boss.stop()
    +    console.log('Initiating graceful shutdown of PgBoss...')
    +    await this.boss.stop({ graceful: true, timeout: gracePeriod })
         console.log('PgBoss stopped successfully')
       } catch (error) {
         console.error('Error stopping PgBoss:', error)
         throw error
       }
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    Why: Implementing a graceful shutdown mechanism is a best practice that ensures ongoing jobs are not abruptly terminated, preventing potential data inconsistencies. This suggestion addresses a critical aspect of system reliability.

    9
    Reliability
    ✅ Implement a retry mechanism for job processing to improve reliability

    Consider adding a retry mechanism for job processing. If a job fails, it might be
    beneficial to retry it a few times before considering it as failed. This can help
    handle temporary issues and improve the overall reliability of the job processing
    system.

    apps/api/src/common/job.handler.ts [27-33]

    -try {
    -  await callback(job.data)
    -  await this.boss.complete(queue, job.id)
    -} catch (error) {
    -  console.error(`Error processing job in queue ${queue}:`, error)
    -  throw error
    +const maxRetries = 3
    +let retries = 0
    +while (retries < maxRetries) {
    +  try {
    +    await callback(job.data)
    +    await this.boss.complete(queue, job.id)
    +    return
    +  } catch (error) {
    +    console.error(`Error processing job in queue ${queue} (attempt ${retries + 1}):`, error)
    +    retries++
    +    if (retries >= maxRetries) {
    +      throw error
    +    }
    +    await new Promise(resolve => setTimeout(resolve, 1000 * retries)) // Exponential backoff
    +  }
     }

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    Why: Implementing a retry mechanism can significantly enhance the reliability of job processing by handling transient errors. This suggestion is relevant and beneficial for improving the robustness of the system.

    8
    Enhancement
    Add a method to check the status of a scheduled job

    Consider adding a method to check the status of a job. This would allow users of the
    JobHandler class to query the status of a specific job, which can be useful for
    monitoring and debugging purposes.

    apps/api/src/common/job.handler.ts [42-54]

     async scheduleJob<V>(
       name: string,
       cron: string,
       data: V,
       options?: PgBoss.ScheduleOptions
    -): Promise<void> {
    +): Promise<string> {
       try {
         const jobId = await this.boss.schedule(name, cron, data, options)
         console.log(`Scheduled job in queue ${name} with ID: ${jobId}`)
    +    return jobId
       } catch (error) {
         console.error(`Error scheduling job in queue ${name}:`, error)
         throw error
       }
     }
     
    +async getJobStatus(jobId: string): Promise<PgBoss.JobWithMetadata | null> {
    +  try {
    +    const job = await this.boss.getJobById(jobId)
    +    return job
    +  } catch (error) {
    +    console.error(`Error getting job status for job ${jobId}:`, error)
    +    throw error
    +  }
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a method to check job status is a useful enhancement for monitoring and debugging. It provides additional functionality that can be valuable for users of the JobHandler class.

    7
    Error handling
    Use specific error types for better error handling and debugging

    Consider using a more specific error type instead of the generic Error in the catch
    blocks. This will provide more detailed information about the nature of the error,
    making it easier to debug and handle specific error scenarios.

    apps/api/src/common/job.handler.ts [15-18]

     } catch (error) {
       console.error('Error starting PgBoss:', error)
    -  throw error
    +  throw new PgBossInitializationError('Failed to start PgBoss', error)
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using specific error types can improve error handling and debugging by providing more context about the error. However, the suggestion requires defining a new error class, which may not be necessary for all use cases.

    5

    💡 Need additional feedback ? start a PR chat

    @Allan2000-Git
    Copy link
    Contributor Author

    @rajdip-b Argument of type V is failing, since it's unknown what type of data is being passed.

    @Allan2000-Git
    Copy link
    Contributor Author

    apps/api/src/common/common.module.ts Outdated Show resolved Hide resolved
    apps/api/src/common/job.handler.ts Outdated Show resolved Hide resolved
    @Allan2000-Git
    Copy link
    Contributor Author

    @rajdip-b A bit help here on why tests are failing. I followed pg-boss documentation and it's says to create an instance using PgBoss class itself.

    @Allan2000-Git Allan2000-Git force-pushed the feat/integrate-pg-boss branch from 499dc08 to a5a371b Compare October 9, 2024 17:42
    @Allan2000-Git Allan2000-Git force-pushed the feat/integrate-pg-boss branch from a5a371b to 6ee16bc Compare October 9, 2024 17:46
    @rajdip-b
    Copy link
    Member

    @rajdip-b A bit help here on why tests are failing. I followed pg-boss documentation and it's says to create an instance using PgBoss class itself.

    Yes, you would need that. What im saying is, you can have that instance of the class in a nestjs provider. you can check how redis is implemented in a similar manner. and you can also look at how the tests work for that.

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Dec 3, 2024

    Hey dude, i'm closing this PR since this seems really obsolete as of now. I'll also be closing the issue. Thanks for grinding the time in here!

    @rajdip-b rajdip-b closed this Dec 3, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Integrate pg-boss for job scheduling
    2 participants