Design proposal for webhooks/notifications #3387

hpolloni · 2022-02-18T00:28:48Z

No description provided.

codecov · 2022-02-18T01:12:22Z

Codecov Report

Merging #3387 (f064791) into main (3bfd0ba) will increase coverage by 0.60%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3387      +/-   ##
============================================
+ Coverage     86.33%   86.93%   +0.60%     
- Complexity     2627     2661      +34     
============================================
  Files           337      341       +4     
  Lines         14799    15022     +223     
  Branches       1115     1108       -7     
============================================
+ Hits          12777    13060     +283     
+ Misses         1613     1569      -44     
+ Partials        409      393      -16

Flag	Coverage Δ
java	`87.41% <ø> (+0.50%)`	⬆️
javascript	`82.55% <ø> (+6.92%)`	⬆️
python	`83.67% <ø> (ø)`

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

Impacted Files	Coverage Δ
...oned/persist/tests/SystemPropertiesConfigurer.java	`57.14% <0.00%> (-10.72%)`	⬇️
...sioned/persist/adapter/DatabaseAdapterFactory.java	`72.00% <0.00%> (-9.82%)`	⬇️
...sist/tests/extension/DatabaseAdapterExtension.java	`64.94% <0.00%> (-0.79%)`	⬇️
...persist/nontx/NonTransactionalDatabaseAdapter.java	`86.82% <0.00%> (-0.48%)`	⬇️
...a/org/projectnessie/deltalake/NessieLogStore.scala	`74.79% <0.00%> (-0.41%)`	⬇️
...src/main/java/org/projectnessie/model/Content.java	`81.81% <0.00%> (ø)`
...c/main/java/org/projectnessie/model/Reference.java	`100.00% <0.00%> (ø)`
.../main/java/org/projectnessie/model/ContentKey.java	`93.54% <0.00%> (ø)`
...n/java/org/projectnessie/services/authz/Check.java	`100.00% <0.00%> (ø)`
...jectnessie/deltalake/NessieLogFileMetaParser.scala	`100.00% <0.00%> (ø)`
... and 89 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bfd0ba...f064791. Read the comment docs.

design/notifications.md

rymurr · 2022-02-21T15:12:06Z

design/notifications.md

+}
+```
+
+Important note is that nessie doesn't make any efforts to de-duplicate the URL and if a particular URL is subscribed


should we de-dup? To me it sounds like we should try

De-dup is tricky thing to implement right. What we can do is to have a deterministic event ID based on the registered endpoint and a hash on the event payload. I think it's better to defer this it to when we know when and how we send out events, whether there's some messaging system in between, whether we synchronously call the event endpoint and the like.

I didn't want to go through that rabbit hole. But the more I think about the more I like the idea of deduping both events and subscriptions. The problem is that we will need to document the dedup rules and I didn't want to get into doing that right now. A simple approach is to have deterministic IDs for subscriptions and/or notifications.

design/notifications.md

rymurr · 2022-02-21T15:17:03Z

design/notifications.md

+TreeApiImpl class with an observer pattern. Every time the specific method in the TreeApi is called, an event is sent
+to the observer. This extension point allows extending to other types of notifications in the future.
+
+### Notification persistence


I don't like this part of the proposal. For other extensiosn to the core we have chosen an SPI and a simple storage mechanism. For example authz has an SPI and the reference impl uses configs provided at runtime. Implementers can then plug in more sophisticated backends with the SPI. Rather than modifying our database adapter and forcing the same storage mechanism i think the SPI gives more flexibility.

agreed that having an SPI would be better and give us/others more flexibility

sounds good. I will take a look

rymurr · 2022-02-21T15:17:54Z

design/notifications.md

+
+## Implementation details
+### Observer extension point
+All the event types that we are interested are associated with the `/tree` API. We propose to extend the current 


It seems that the simplest approach is the same as the auth and tracing idioms: create a wrapper class. This avoids having to make significant changes to the kernel

nastra

overall looks good, just a few comments

design/notifications.md

nastra · 2022-02-22T07:47:06Z

design/notifications.md

+
+## Implementation details
+### Observer extension point
+All the event types that we are interested are associated with the `/tree` API. We propose to extend the current 


nastra · 2022-02-22T07:48:29Z

design/notifications.md

+TreeApiImpl class with an observer pattern. Every time the specific method in the TreeApi is called, an event is sent
+to the observer. This extension point allows extending to other types of notifications in the future.
+
+### Notification persistence


agreed that having an SPI would be better and give us/others more flexibility

design/notifications.md

snazy

The description of events LGTM - just a few comments about details.

But I'd prefer to move the management and especially the implementation details out of this document. Maybe we can start with a prototype that synchronously calls a statically configured endpoint. Maybe we tend to push events to a messaging system and have dedicated workers that are responsible for actually invoking the registered endpoint(s), deal with retries, error conditions, etc. And then think how we can actually implement a full-blown notification management system.

snazy · 2022-02-22T09:46:04Z

design/notifications.md

+      type: string
+      format: date
+      description: The time the event was fired in ISO date format.
+    fromRefName:


Can use fromRef here w/ type Reference here as well - just with a note that the hash field in there will always be
null.

design/notifications.md

snazy · 2022-02-22T09:54:52Z

design/notifications.md

+   1. Its content-type is `application/json`.
+   2. Its body contains the JSON payload of the event that happened. (See previous section for details on the expected schema).
+
+### Creating a new notification/webhook


I think, event management and event description should be separate things. So one doc that describes the events (as above). Not sure whether we should have a full blown notification management system here. Maybe it's initially sufficient to have a notification endpoint configured via Quarkus.

snazy · 2022-02-22T09:58:28Z

design/notifications.md

+}
+```
+
+Important note is that nessie doesn't make any efforts to de-duplicate the URL and if a particular URL is subscribed


De-dup is tricky thing to implement right. What we can do is to have a deterministic event ID based on the registered endpoint and a hash on the event payload. I think it's better to defer this it to when we know when and how we send out events, whether there's some messaging system in between, whether we synchronously call the event endpoint and the like.

snazy · 2022-02-22T09:59:27Z

design/notifications.md

+## Implementation details
+### Observer extension point
+All the event types that we are interested are associated with the `/tree` API. We propose to extend the current 
+TreeApiImpl class with an observer pattern. Every time the specific method in the TreeApi is called, an event is sent


Nit: let's leave out implementation classes here.

sounds good. Both Ryan and Eduard had concerns on this part of the proposal. Let's talk offline

design/notifications.md

Co-authored-by: Robert Stupp <snazy@snazy.de>

Co-authored-by: Ryan Murray <rymurr@gmail.com>

dimas-b · 2023-02-22T20:22:02Z

This PR has been dormant for about a year - closing. Expect another PR for notifications soon (with different design choices).

Design proposal for webhooks/notifications

c13d812

rymurr reviewed Feb 21, 2022

View reviewed changes

nastra reviewed Feb 22, 2022

View reviewed changes

snazy reviewed Feb 22, 2022

View reviewed changes

design/notifications.md Show resolved Hide resolved

snazy reviewed Feb 22, 2022

View reviewed changes

Herman Polloni and others added 2 commits February 22, 2022 08:49

Update design/notifications.md

b7d771c

Co-authored-by: Robert Stupp <snazy@snazy.de>

Update design/notifications.md

f064791

Co-authored-by: Ryan Murray <rymurr@gmail.com>

ajantha-bhat mentioned this pull request Feb 25, 2022

Nessie SQL extensions: Fix Use reference at hash #3440

Merged

dimas-b closed this Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Design proposal for webhooks/notifications #3387

Design proposal for webhooks/notifications #3387

hpolloni commented Feb 18, 2022

codecov bot commented Feb 18, 2022 •

edited

Loading

rymurr Feb 21, 2022

snazy Feb 22, 2022

hpolloni Feb 22, 2022

rymurr Feb 21, 2022

nastra Feb 22, 2022

hpolloni Feb 22, 2022

rymurr Feb 21, 2022

nastra Feb 22, 2022

nastra left a comment

nastra Feb 22, 2022

nastra Feb 22, 2022

snazy left a comment

snazy Feb 22, 2022

snazy Feb 22, 2022

snazy Feb 22, 2022

snazy Feb 22, 2022

hpolloni Feb 22, 2022

dimas-b commented Feb 22, 2023

Design proposal for webhooks/notifications #3387

Design proposal for webhooks/notifications #3387

Conversation

hpolloni commented Feb 18, 2022

codecov bot commented Feb 18, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snazy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b commented Feb 22, 2023

codecov bot commented Feb 18, 2022 •

edited

Loading