-
Notifications
You must be signed in to change notification settings - Fork 1
Watcher and subscription protocols #104
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
Conversation
…ature/integrated-server
(fluree/commit! ledger staged-db {:file-data? true})) | ||
broadcast-msg (events/transaction-committed params commit-result)] | ||
result (deref! | ||
(fluree/apply-stage! ledger staged-db)) |
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.
What is apply-stage
do here? When I try to load it my repl it says no definitions found.
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.
Ok, I found the branch, I'm not sure why xref
wasn't working for me. So it looks like apply-stage
is just commit with a wrapper.
I'm kind of ambivalent about having this as a top-level api - we call directly into the connection
ns in other places, could we do that here too instead of making apply-stage
top-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 used to be a top level api because it was part of the commit!
function. You had to pass the :file-data? true
option to get the output to have this shape. I don't think we should be doing this kind of control flow with options, so I split that functionality up into two separate functions. I made it a top level api because that functionality used to be a top level api, and I can see other users wanting to know the commit address after calling the commit.
; :ledgers {"some/ledger" {}} ;; <- map where each key is a ledger-id subscribed to | ||
; :did nil}} | ||
; :closed? false} | ||
(defprotocol Publicizer |
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 is minor, but I feel like this "publishes" rather than "publicizes" and would maybe be better with the name Publisher
, publish-commit
, and publish-new-ledger
. I don't insist on it, though : )
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.
We already have a Publisher
protocol that the name services implement, so I didn't want to reuse that term.
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 patch adds protocols for watcher and subscription functionality to allow us to more flexibly publicize new commits as well as report on the status of pending transactions.
This patch also adds a new type of standalone transactor that does not terminate it's return channel until the transaction is fully processed instead of maintaining a buffer of pending transactions. This new transactor will apply back pressure to down stream callers instead of relying on those callers to detect and manage their submitted transactions from errors returned.
This patch lastly removes the commit and data files from consensus messages in favor of just including the commit address so receivers can download the files themselves from storage as necessary. This temporarily breaks Raft consensus, but this will be fixed in a subsequent patch.
Lastly, this patch refactors the application route initialization to allow us to (eventually) initialize routes more dynamically, and adds a callback route to allow delegated transactors to report the status of pending transactions they're processing.