-
Notifications
You must be signed in to change notification settings - Fork 1
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 #13
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.
A few small logging things to consider / clean up. But very minor and looks good overall!
|
||
(defmethod client-message :on-bytes | ||
[{:keys [http/sub-id payload offset len]}] | ||
(log/info "websocket :on-bytes (no-op) message for sub-id: " sub-id)) |
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.
The log fns will insert the spaces between args for you (like println
)
|
||
(defmethod client-message :on-error | ||
[{:keys [http/sub-id error]}] | ||
(log/warn "Websocket error for sub-id: " sub-id "with error: " (type error))) |
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 we log this at error
level? Or are they so common that warn
is better?
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.
Very common, just a network connection break. There is a list of error types, and I'd suspect the optimal solution will be to eventually case
the error type, with some being log/debug all the way up to log/error. So log/warn is sort of an imperfect compromise. I think we'll have a better sense of what is best here once it is running.
"Sends a message to all subscriptions. Message sent as JSON stringified map | ||
in the form: {action: commit, ledger: my/ledger, data: {...}}" | ||
[{:keys [sub-atom] :as subscriptions} action ledger-alias data] | ||
(log/warn "SEND MESSAGE TO ALL subscriptions: " subscriptions) |
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.
Why is this logged at warn
level?
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.
This was a debugging message, I'll remove.
# Conflicts: # deps.edn # test/fluree/server/integration/policy_test.clj
…x-update # Conflicts: # deps.edn
We need to preserve the txn context in order to correctly expand the insert and delete clauses later. Expansion removes the context, so we need to pass it along as an opt instead of as part of the txn document.
Updated to the latest fluree/db so it fixes the failing policy test, it also gets some commits that fixed the where syntax. |
This is companion support for
fluree/db
subscriptions: fluree/db#605When connecting to a fluree/server, the fluree/db library now launches a socket connection to keep up to date on ledger changes and maintains its internal state and a current copy of db's that are actively being used from the library.