-
Notifications
You must be signed in to change notification settings - Fork 0
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/persist webhook events #24
base: master
Are you sure you want to change the base?
Conversation
3d3e173
to
8b89465
Compare
src/main/java/com/impactupgrade/nucleus/controller/StripeController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/controller/StripeController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/dao/FailedRequestDao.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/dao/FailedRequestMongoDbDao.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/service/logic/FailedRequestService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/impactupgrade/nucleus/service/logic/FailedRequestService.java
Outdated
Show resolved
Hide resolved
5b7b368
to
e6d985e
Compare
@Setter | ||
@Entity | ||
@Table(name = "failed_request", schema = "public") | ||
public class FailedRequest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VSydor Re: what we were talking about yesterday, wondering if we should refactor this to make it more of a WebhookRequest
class. I think the fields can be kept the same, but we'd switch to inserting this at the beginning of every single webhook Controller method. If processing fails, update it with the errorMessage? If it succeeds, delete it? Or possibly retain it for 7ish days in case we need to analyze it?
That will get us one step closer to creating a durable, async queue...
Thoughts? CC @LandonBaer721
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I see it:
if we want to keep track of every single incoming event - then we should do it like you said: persist on each request and then update with an error message if processing failed (and keep "successful events" for some longer time. But how do we clean-up them later on? Will have to do it manually in some way?).
if the idea is to track only failed-to-process events - then there is no need to persist every event - only failed ones (and then delete them after successful processing?)
Please let me know which approach is preferred - I'll update the code.
} catch (Exception e) { | ||
log.error("failed to process the Stripe event", e); | ||
// TODO: email notification? | ||
log.info("Saving event with id '{}' as failed request...", event.getId()); | ||
env.failedRequestService().persist(event.getId(), new JSONObject(event), e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IE, shift this to the beginning of the webhook
method, then replace this line with an update to mark it as failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update
e6d985e
to
c1d6ad1
Compare
c1d6ad1
to
e822f80
Compare
No description provided.