Skip to content

Conversation

@beelis
Copy link
Contributor

@beelis beelis commented Mar 25, 2025

Fixes #24121

Motivation

When an error occurs during the sending of a message using NewOutputMessage, the go function exits with a panic. This behavior prevents the application from handling the error gracefully, as it bypasses any error-handling logic.

Errors should be returned by the new introduced func NewOutputMessageWithError so they can be handled by the calling code, rather than causing a panic.

Modifications

Return the error on NewOutputMessageWithError

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: https://github.com/beelis/pulsar/tree/enhancements/24121/Return-error-on-NewOutputMessage

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 25, 2025
@beelis beelis changed the title [Enhancement][functions go] Return error on NewOutputMessage to enable error handling [improvement][functions go] Return error on NewOutputMessage to enable error handling Mar 25, 2025
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

This PR breaks the public API.

Could you consider that add the NewOutputMessageWithError to the FunctionContext?

@beelis beelis force-pushed the enhancements/24121/Return-error-on-NewOutputMessage branch from 6a09b3e to 8111741 Compare March 26, 2025 10:11
@beelis beelis changed the title [improvement][functions go] Return error on NewOutputMessage to enable error handling [improvement][functions go] Introduce NewOutputMessageWithError to enable error handling Mar 26, 2025
@beelis
Copy link
Contributor Author

beelis commented Mar 26, 2025

Hi @nodece
Thank you for your review and feedback, i introduced the new func NewOutputMessageWithError.
Cheers, Simon

@beelis beelis requested a review from nodece March 26, 2025 10:14
@beelis beelis force-pushed the enhancements/24121/Return-error-on-NewOutputMessage branch from 593df9e to 8461215 Compare March 28, 2025 08:15
@beelis beelis requested a review from nodece March 28, 2025 08:16
Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

Please check.

Co-authored-by: Zixuan Liu <[email protected]>
@beelis beelis requested a review from nodece March 28, 2025 08:44
@nodece nodece requested a review from Copilot March 28, 2025 09:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a new error-handling function, NewOutputMessageWithError, to allow graceful error handling when sending output messages in the Go Pulsar function implementation.

  • Add a new function in both instance.go and context.go to return an error instead of panicking.
  • Update tests in context_test.go to cover the new function.
  • Minor refactoring in util_test.go to improve variable naming.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
pulsar-function-go/pf/util_test.go Corrected a typo in variable naming from actualtMap to actualMap.
pulsar-function-go/pf/instance.go Introduced a new outputMessageWithError function to support error handling.
pulsar-function-go/pf/context_test.go Added test cases for the new NewOutputMessageWithError functionality.
pulsar-function-go/pf/context.go Added a new field and method for NewOutputMessageWithError.
Comments suppressed due to low confidence (2)

pulsar-function-go/pf/instance.go:83

  • [nitpick] Consider adding a space after 'error is:' in the error message for improved readability, for example: "getting producer failed, error is: %v".
log.Errorf("getting producer failed, error is:%v", err)

pulsar-function-go/pf/context_test.go:126

  • Comparing errors by value can lead to false negatives since two errors with the same message may not be equal if they are distinct instances. Consider using assert.EqualError(t, err, "test error") or comparing error messages.
assert.Equal(t, testCase.expectedError, err)

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, good work @beelis

@lhotari lhotari added this to the 4.1.0 milestone Mar 28, 2025
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.24%. Comparing base (bbc6224) to head (c288ae0).
Report is 999 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24122      +/-   ##
============================================
+ Coverage     73.57%   74.24%   +0.66%     
+ Complexity    32624    32484     -140     
============================================
  Files          1877     1864      -13     
  Lines        139502   144453    +4951     
  Branches      15299    16479    +1180     
============================================
+ Hits         102638   107244    +4606     
+ Misses        28908    28747     -161     
- Partials       7956     8462     +506     
Flag Coverage Δ
inttests 26.68% <ø> (+2.10%) ⬆️
systests 23.19% <ø> (-1.14%) ⬇️
unittests 73.74% <ø> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1068 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodece nodece changed the title [improvement][functions go] Introduce NewOutputMessageWithError to enable error handling [improve][fn] Introduce NewOutputMessageWithError to enable error handling Mar 28, 2025
@nodece nodece merged commit d8e2743 into apache:master Mar 28, 2025
55 checks passed
lhotari pushed a commit that referenced this pull request Mar 28, 2025
…dling (#24122)

Co-authored-by: Simon Beeli <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit d8e2743)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 3, 2025
…dling (apache#24122)

Co-authored-by: Simon Beeli <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit d8e2743)
(cherry picked from commit d828bc6)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 3, 2025
…dling (apache#24122)

Co-authored-by: Simon Beeli <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit d8e2743)
(cherry picked from commit d828bc6)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 8, 2025
…dling (apache#24122)

Co-authored-by: Simon Beeli <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit d8e2743)
(cherry picked from commit d828bc6)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 8, 2025
…dling (apache#24122)

Co-authored-by: Simon Beeli <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
(cherry picked from commit d8e2743)
(cherry picked from commit d828bc6)
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Pulsar Functions Go: Return error on NewOutputMessage to enable error handling

4 participants