Skip to content

ensure cron tasks get added to cache#222

Merged
jbsmith7741 merged 1 commit intopcelvng:mainfrom
jbsmith7741:flowlord-cache
Jan 10, 2025
Merged

ensure cron tasks get added to cache#222
jbsmith7741 merged 1 commit intopcelvng:mainfrom
jbsmith7741:flowlord-cache

Conversation

@jbsmith7741
Copy link
Collaborator

@jbsmith7741 jbsmith7741 commented Dec 17, 2024

PR Type

Enhancement


Description

  • Added new cache integration for task producer that automatically caches tasks before sending
  • Refactored Cronjob and batchJob implementations to use the new cached sending mechanism
  • Improved error handling by sending failed tasks to alerts channel
  • Simplified code structure by moving alerts channel to Cronjob struct

Changes walkthrough 📝

Relevant files
Enhancement
producer.go
Add cache integration for task producer                                   

apps/flowlord/cache/producer.go

  • Added new SendFunc method to Memory cache that wraps a producer's send
    function
  • Automatically adds tasks to cache before sending them via the producer

  • +14/-0   
    job.go
    Refactor job handling to use cached task sending                 

    apps/flowlord/job.go

  • Replaced direct producer usage with sendFunc wrapper in Cronjob struct
  • Updated error handling to send failed tasks to alerts channel
  • Refactored batchJob to use new sendFunc for task sending
  • +13/-10 

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

    @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling
    The SendFunc method doesn't check if the cache Add operation was successful before proceeding with sending the task

    Resource Leak
    The alerts channel usage in batchJob.Run could potentially block if the channel is full or not being consumed

    Code Duplication
    Error handling logic is duplicated between the initial task error check and the loop error handling in batchJob.Run

    @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add nil pointer check to prevent application crashes

    Check for nil task pointer before dereferencing in SendFunc to prevent potential
    panic.

    apps/flowlord/cache/producer.go [10-13]

     return func(topic string, tsk *task.Task) error {
    +    if tsk == nil {
    +        return fmt.Errorf("cannot send nil task")
    +    }
         m.Add(*tsk)
         return p.Send(topic, tsk.JSONBytes())
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical defensive programming practice that prevents potential runtime panics from nil pointer dereference, which could crash the application. The suggestion provides proper error handling for this case.

    9
    Add null check for callback function to prevent runtime panic

    Check for nil sendFunc before using it in Cronjob.Run() to prevent panic if the
    function wasn't properly initialized.

    apps/flowlord/job.go [48-52]

    +if j.sendFunc == nil {
    +    tsk.Result = task.ErrResult
    +    tsk.Msg = "sendFunc not initialized"
    +    j.alerts <- *tsk
    +    return
    +}
     if err := j.sendFunc(j.Topic, tsk); err != nil {
         tsk.Result = task.ErrResult
         tsk.Msg = err.Error()
         j.alerts <- *tsk
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion prevents a potential runtime panic by checking for uninitialized sendFunc, which is a critical safety check since the function is injected as a dependency. The error handling properly reports the issue through the alerts channel.

    9
    Add error handling for cache operations to prevent silent failures

    Add error handling for the Add operation in the SendFunc method. If adding to cache
    fails, it should be reported before attempting to send the task.

    apps/flowlord/cache/producer.go [10-13]

     return func(topic string, tsk *task.Task) error {
    -    m.Add(*tsk)
    +    if err := m.Add(*tsk); err != nil {
    +        return err
    +    }
         return p.Send(topic, tsk.JSONBytes())
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a significant error handling gap where cache failures could be silently ignored, potentially leading to data inconsistency. Adding proper error handling is crucial for system reliability.

    8

    @jbsmith7741 jbsmith7741 force-pushed the flowlord-cache branch 3 times, most recently from b47d422 to e966604 Compare December 19, 2024 23:18
    @jbsmith7741 jbsmith7741 requested a review from zJeremiah January 3, 2025 22:57
    @jbsmith7741 jbsmith7741 merged commit 6e2c655 into pcelvng:main Jan 10, 2025
    @jbsmith7741 jbsmith7741 deleted the flowlord-cache branch January 10, 2025 20:42
    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.

    2 participants