Skip to content

Feature/subscription #605

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

Merged
merged 11 commits into from
Oct 26, 2023
Merged

Feature/subscription #605

merged 11 commits into from
Oct 26, 2023

Conversation

bplatz
Copy link
Contributor

@bplatz bplatz commented Oct 25, 2023

This PR continues work on #431 :

  • Adds round-trip subscriptions, so fluree/server can be started as a query peer and always stay updated with latest DB changes
  • Adds a new fluree/notify API, that allows any external nameservice to "notify" a connection with a new commit, and the cached DB will be updated (if consistent with prior state)
  • Caches loaded ledgers, and therefore current DBs, so fluree/load only goes to disk if ledger hasn't been used before
  • Eliminates a race condition that existed where the same 'ledger' can exist as different in different objects and have inconsistent state
  • Removes some of the Thread/sleep & retry hacks previously built into tests that were in part an artifact of the above race conditions (these should all be able to be removed now as tests are getting updated)

@bplatz bplatz requested a review from a team October 25, 2023 19:16
Copy link
Contributor

@zonotope zonotope left a comment

Choose a reason for hiding this comment

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

I only have a few very minor questions/suggestions, but this looks great to me.

:timeout timeout
:close (fn [_ code reason]
(log/debug "Websocket closed; code" code
(let [ws-config {:connect-timeout timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely not for this pr, but I think stuff like this could work nicely with a framework like donut to manage this config and make it more flexible and pluggable.

;; missing some updates, dump in-memory ledger forcing a reload
(> commit-t (inc latest-t))
(do
(close-ledger ledger)
Copy link
Contributor

Choose a reason for hiding this comment

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

super minor, but I think a debug log would be good here since we're changing internal ledger state. it might make it easier to figure stuff out if we had to, well, debug an issue later.

(when-let [path (->> address
(re-matches #"^fluree:[^:]+://(.*)$")
(second))]
(if (str/ends-with? path "/main/head")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a TODO comment here could be useful to let us know we have to tighten this up later.

(defn record-ledger-subscription
[{:keys [server-state] :as _conn} ledger-id]
(swap! server-state assoc-in [:subscriptions ledger-id] {:subscribed-at (util/current-time-millis)}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Super duper minor, but this returns the whole server state. it probably doesn't matter, but maybe we want to return just the bit added about the specific ledger we're subscribing to.

(defn request-ledger-subscribe
[conn ledger-id]
#_(conn-proto/-msg-out conn {:action :subscribe
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be commented? if so, then a TODO about why could be useful.

@bplatz bplatz merged commit 235f222 into main Oct 26, 2023
@bplatz bplatz deleted the feature/subscription branch October 26, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants