-
Notifications
You must be signed in to change notification settings - Fork 23
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
Feature/subscription #605
Conversation
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.
I only have a few very minor questions/suggestions, but this looks great to me.
src/fluree/db/util/xhttp.cljc
Outdated
:timeout timeout | ||
:close (fn [_ code reason] | ||
(log/debug "Websocket closed; code" code | ||
(let [ws-config {:connect-timeout timeout |
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.
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) |
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.
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") |
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.
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)})) |
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.
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 |
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.
should these be commented? if so, then a TODO about why could be useful.
This PR continues work on #431 :
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)fluree/load
only goes to disk if ledger hasn't been used before