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

Feature/pub 71 add error handler #75

Merged
merged 3 commits into from Oct 5, 2021

Conversation

ketan-saxena
Copy link
Contributor

PUB-71:

  • Add handleError in Subscription class
  • Add handleError method in SubscriberOptions for SubscriptionService
  • Update example subscription

@ketan-saxena ketan-saxena marked this pull request as ready for review September 30, 2021 23:55
.changeset/real-nails-fetch.md Outdated Show resolved Hide resolved
src/subscriber/subscriber.ts Outdated Show resolved Hide resolved
src/subscriber/subscriberV2.ts Outdated Show resolved Hide resolved
src/subscriber/subscriberV2.ts Outdated Show resolved Hide resolved
src/client/googlePubSub.ts Outdated Show resolved Hide resolved
@rohit-gohri rohit-gohri added the enhancement New feature or request label Oct 1, 2021
Copy link
Member

@rohit-gohri rohit-gohri left a comment

Choose a reason for hiding this comment

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

LGTM with some nitpicks regarding defaults

Comment on lines +48 to +51
public static handleError(error: Error): void {
// default error handling logic
Logger.Instance.error({ error }, 'Received Unexpected Error');
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's rethrow the error in the default handler, so application can handle it on their own

Suggested change
public static handleError(error: Error): void {
// default error handling logic
Logger.Instance.error({ error }, 'Received Unexpected Error');
}
public static handleError(error: Error): void {
// default error handling logic
Logger.Instance.error({ error }, 'Received Unexpected Error');
throw error;
}

Comment on lines +30 to +32
handleError: function(error: Error): void {
// internal error handling logic for subscriber
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's add an example similar to google docs: https://googleapis.dev/nodejs/pubsub/latest/index.html#using-the-client-library

Suggested change
handleError: function(error: Error): void {
// internal error handling logic for subscriber
}
handleError: function(error: Error): void {
// internal error handling logic for subscriber
console.error('Subscription error', error);
// shutdown any connections here and exit
process.exit(1);
}

Similar handlers in other examples also

@ishan123456789 ishan123456789 merged commit 4f45811 into main Oct 5, 2021
@ishan123456789 ishan123456789 deleted the feature/PUB-71-add-error-handler branch October 5, 2021 09:45
@github-actions github-actions bot mentioned this pull request Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants