Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

feat: batching events and webhook plugin support #1359

Merged
merged 11 commits into from
Jul 7, 2023
Merged

feat: batching events and webhook plugin support #1359

merged 11 commits into from
Jul 7, 2023

Conversation

EnriqueL8
Copy link
Contributor

@EnriqueL8 EnriqueL8 commented Jun 29, 2023 •
edited by peterbroadhurst
Loading

Enable batching for subscriptions

  • Write more robust tests to match coverage
  • Handle the firstData for events and decide what we do there
  • Refactor the webhook plugin to make it more modular instead of copying code across functions

@EnriqueL8
feat: batching events and webhook plugin support
baccf3f 
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023 •
edited
Loading

Codecov Report

Merging #1359 (be50b97) into main (816463c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##             main    #1359    +/-   ##
========================================
  Coverage   99.98%   99.98%            
========================================
  Files         315      315            
  Lines       21907    22036   +129     
========================================
+ Hits        21903    22032   +129     
  Misses          2        2            
  Partials        2        2
Impacted Files Coverage Δ
pkg/core/event.go 100.00% <ø> (ø)
pkg/core/subscription.go 100.00% <ø> (ø)
internal/events/event_dispatcher.go 100.00% <100.00%> (ø)
internal/events/event_manager.go 100.00% <100.00%> (ø)
internal/events/subscription_manager.go 100.00% <100.00%> (ø)
internal/events/system/events.go 100.00% <100.00%> (ø)
internal/events/webhooks/webhooks.go 100.00% <100.00%> (ø)
internal/events/websockets/websockets.go 100.00% <100.00%> (ø)
internal/orchestrator/subscriptions.go 100.00% <100.00%> (ø)

@EnriqueL8
Copy link
Contributor Author

PR not ready for review as I need to work on removing the nack I added and test the infinite retry is now working

@EnriqueL8 EnriqueL8 force-pushed the batching_webhooks branch from fe72060 to baccf3f Compare July 6, 2023 13:28
@EnriqueL8
test: testing batching part 1
07a12d9 
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
EnriqueL8 added 3 commits July 6, 2023 21:13
@EnriqueL8
refactor into a create body function
6160d87 
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@EnriqueL8
one more refactor of body creation
d62b758 
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@EnriqueL8
more refactoring the refactoring
74c4d5d 
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@EnriqueL8 EnriqueL8 marked this pull request as ready for review July 6, 2023 21:46
EnriqueL8 and others added 5 commits July 6, 2023 23:32
@EnriqueL8
fix: creating of body and check withData with batch are incompatible
c055e6b 
Signed-off-by: Enrique Lacal <enrique.lacal@kaleido.io>
@peterbroadhurst
Tweaks to processing to simplfy and deferring parsing
eb028bd 
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Additional removal of code duplication and test coverage
4ef4f39 
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Merge branch 'main' of github.com:hyperledger/firefly into batching_w…
0efdf34 
…ebhooks

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst
Prevent creation of batch subscription on transport without support
d579f58 
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
peterbroadhurst
peterbroadhurst approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

@EnriqueL8 - looked great - I've filled in a couple of commits to simplify a bit of duplicate logic, and found an efficiency in deferring parsing of some data from string to JSON.

Also made the change to prevent creation of subscriptions with batching enabled, for event transports that don't support it.

@peterbroadhurst peterbroadhurst marked this pull request as draft July 7, 2023 14:00
@peterbroadhurst
Copy link
Contributor

Popped back to draft as there's a potential issue with creating subscriptions with the default transport

@peterbroadhurst
Move setting the default to earlier in the flow, before valdiation
be50b97 
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@EnriqueL8 EnriqueL8 marked this pull request as ready for review July 7, 2023 16:18
@EnriqueL8
Copy link
Contributor Author

Tested the above fix from Peter and all good you can create a subscription with a transport and it defaults to web hooks

@EnriqueL8 EnriqueL8 merged commit 4f5c029 into hyperledger:main Jul 7, 2023
@peterbroadhurst peterbroadhurst mentioned this pull request Jan 15, 2024
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@peterbroadhurst peterbroadhurst peterbroadhurst approved these changes

@nguyer nguyer Awaiting requested review from nguyer

@awrichar awrichar Awaiting requested review from awrichar

@shorsher shorsher Awaiting requested review from shorsher

@nickgaski nickgaski Awaiting requested review from nickgaski

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EnriqueL8 @codecov-commenter @peterbroadhurst