Overhaul the Khepri API again #145
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportBase: 91.55% // Head: 90.96% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #145 +/- ##
==========================================
- Coverage 91.55% 90.96% -0.59%
==========================================
Files 14 16 +2
Lines 2617 2868 +251
==========================================
+ Hits 2396 2609 +213
- Misses 221 259 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
the-mikedavis
requested changes
Oct 3, 2022
|
I see I forgot to update the |
dumbbell
force-pushed
the
api-overhaul
branch
3 times, most recently
from
4f3f182 to
09ed2db
Compare
I was still not satisfied with the previous revisit in commit 3fc0da9. I found the return values to be too complicated for the most common use cases. In this commit, the `khepri` and `khepri_tx` modules expose the same simple API: * `khepri:get()` returns `{ok, Payload}` directly. If the node has no payload, `{ok, undefined}` is returned. It's possible to use `khepri:get_or()` to get a default value instead of `undefined`. * `khepri:get_many()` and `khepri:get_many_or()` are introduced to get many nodes in a single call. The result is of the form `{ok, #{Path => Payload}}`. * Writes (`khepri:put()`, `khepri:delete()`...) always return `ok` in case of success. * There are also `khepri:put_many()` and `khepri:delete_many()` to provide the same distinctions as `khepri:get()` and `khepri:get_many()`. The detailed return values are still available from the `khepri_adv` and `khepri_tx_adv` modules. They expose the same functions as their simple API counterparts, but return complete values like before this commit. There are few small changes I want to make that will come in follow-up commits.
Before this change, functions such as `khepri_adv:put()` would return the properties and payload of a node as they were before the update. With this patch, the payload is still the old one, however other properties such as the payload version correspond to the node after the update. This allows to get the payload version of a freshly modified tree node without having to query it.
This simplifies the API and the code. `keep_while` is extracted by `khepri_machine` when needed, like it does for command and tree options.
With this commit, all errors and exceptions reported by Khepri should
all have the same form:
{khepri | khepri_ex, Name, Props}
Errors are returned by the Khepri API to notify the failure of an
operation that is to be expected in a correct use of the library. Things
like a missing tree node or a non-matching path pattern. They are
expected to be handled by the caller. Here is an example of an error:
{error, {khepri, node_not_found, #{...}}} = khepri:get(...)
Exceptions are raised by Khepri using `erlang:error()` to notify an
abnormal use of the library. Things like incorrect arguments or the
failure to prepare a transaction function. They are not expected to be
handled by the caller, they mostly target the developer. Here is an
example of an exception:
try
khepri:get([?STAR])
catch
error:{khepri_ex, possibly_matching_many_nodes_denied, #{...}} ->
...
end
Some functions will return errors using `erlang:throw()` because their
return value doesn't leave room to return Khepri errors. This is the
case for instance with `khepri:run_sproc(...)` which returns directly
the value of the executed stored procedure otherwise.
Otherwise, they are a bit too generic and could cause conflicts with other libraries out there.
This avoids namespace pollution. We export the type at the same time.
"Properties" is the name we use everywhere. Let's make that consistent the record's field name consistent with the rest.
This breaks the clustering API, but anyway, the branch currently breaks everything.
... instead of `lists:reverse/1` plus `++`. This should be slightly more efficient.
The following bang functions were missing: * `get_or!` * `get_many!` * `get_many_or!` * `exists!` * `has_data!` * `is_sproc!` * `count!` * `put_many!` * `delete_many!` * `delete_payload!` * `delete_many_payloads!`
`re_mp()` was clear what it was only for me apparently :) While renaming the type to something more human-friendly nad making it more precise, I discovered that the specs were buggy (because of the lack of precision before). In the end, instead of having a type for the compiled regex, the type now represents the return value of `re:compile/1`.
the-mikedavis
approved these changes
Oct 5, 2022
dumbbell
added a commit
to rabbitmq/khepri-benchmark
that referenced
this pull request
Oct 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was still not satisfied with the previous revisit in #79. I found the return values to be too complicated for the most common use cases.
The
khepriandkhepri_txmodules expose the same simple API:khepri:get()returns{ok, Payload}directly. If the node has no payload,{ok, undefined}is returned. It's possible to usekhepri:get_or()to get a default value instead ofundefined.khepri:get_many()andkhepri:get_many_or()are introduced to get many nodes in a single call. The result is of the form{ok, #{Path => Payload}}.khepri:put(),khepri:delete()...) always returnokin case of success.khepri:put_many()andkhepri:delete_many()to provide the same distinctions askhepri:get()andkhepri:get_many().Example of the new simple API:
The detailed return values are still available from the
khepri_advandkhepri_tx_advmodules. They expose the same functions as their simple API counterparts, but return complete values like before this commit. Also, thekhepri*_adv:put()-related functions now return the properties of the tree node after the update, but the payload as it was before the update. This allows to know immediately the payload version of an updated tree node.Example of the new advanced API:
Erros have been sanitized a bit:
{error, ...}tuples) are returned for "normal" error conditions, like a missing tree node. The only exception to that where the error is thrown usingerlang:throw/1is when the function returns an arbitrary value (like withkhepri:run_sproc()) and doesn't have room for Khepri-level errors.{khepri, Name, Props}whereNameis an atom qualifying the error andPropsis a map to add more details to the error. That way, it's easier to understand where the error comes from when Khepri is used indirectly through a pile of other applications for instance.erlang:error/1) are thrown when there is a misuse of the library.{khepri_ex, Name, Props}.NameandPropshave the same meaning as for errors.Example of an error:
{error, {khepri, node_not_found, #{node_name := non_existent, node_path := [non_existent], node_is_target := true}}} = khepri:get("/:non_existent").Example of an exception:
The public header,
include/khepri.hrlhas been fixed to avoid namespace pollution and conflicts with other libraries by using theKHEPRI_prefix in all exposed macros.Some of the renamed macros:
This patch is nearly impossible to review, given the heavy code churn. I'm mostly looking for feedback on the public API, even if we discussed it several times internally.