Skip to content
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

Introductory PR for lots of changes #31

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

rrichardson
Copy link

@rrichardson rrichardson commented Nov 1, 2022

What problem are we solving?

I have been building some systems against this library for the last couple of days and made a bunch of changes to my fork. These changes include additional functions, but also updates the rs-consul code to make it more "idiomatic Rust".

A couple of these changes do break backwards compatibility a bit, but not a lot. This would probably warrant a new minor version.

This is a lot of changes, so it might need to be broken up into a few separate PRs, which I'm willing to do. I'd just like to put this out there to get some feedback first.

Here is an incomplete list of changes:

Introduced a Base64Vec type

This is mostly used under the hood to simplify sending Vec<u8> to and from Consul. It transparently base64 encodes and decodes with a custom Serializer and Deserializer.
ReadKeyRequest is generic over T (see below) but defaults to Base64Vec.

Changed all KV related operations to be generic over type T

Previously one had to write Vec<u8> and read a String, which I found to be a little awkward.
In all cases, T must implement Serialize or Deserialize + Debug + Default and in some cases (get_lock) it requres Clone as well.
If all you're doing is storing and retrieving String this interface should be a bit simpler, because you don't have to convert to and from Vec

Methods updated to be Generic over T:

  • create_or_update_key
  • read_key
  • get_lock
  • get_lock_inner

Structs updated to be generic over T:

  • LockGuard (was Lock)
  • ReadKeyResponse

Added Transaction operations

You can now send batches of operations to be executed with (some?) isolation using Consuls txn API. (see https://developer.hashicorp.com/consul/api-docs/txn)

This includes new a new method: create_or_update_all which takes a Vec of TransactionOp. Note that I didn't add support for all request types into TransactionOp, only KV, which is all that I needed.

Note that TransactionOp is not generic over T, because A Vec<TransactionOp<T>> would allow only 1 type of value to be written. So it takes a Base64Vec as a value instead.

Tests are idempotent, for the most part

The tests assumed that the database was pristine, so some tests would fail on multiple runs. This is no longer the case. They now clean up before their operations.

Exposed create_session and made a get_lock_inner

For those that don't want to use a LockGuard with drop semantics, because they have their own system for managing locks, I've exposed the inner workings of get_lock

Made everything more self-consistent and idiomatic Rust

  • Some of the Requests were called *Payload now they're all called *Request
  • Some requests took Strings while others took &str - Now they all take &str
  • Some structs had members which were PascalCase and had an allow attr. These have been changed to snake_case, and use serde_rename = "PascalCase" instead, like the other structs.

Checks

Please check these off before promoting the pull request to non-draft status.

  • All CI checks are green.
  • I have reviewed the proposed changes myself.

@kushudai
Copy link
Contributor

Hi @rrichardson
Thank you for all this work!
I see you maintain a pretty elaborate fork of this library.

We would love to integrate your changes but the scope of them is quite large and a bit risky to pull off all in one go (since we use this library internally).

If you could sign the CLA, I would be happy to try to pull these changes in piecemeal.

@github-actions
Copy link

github-actions bot commented Jun 26, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@cholcombe973
Copy link

I have read the CLA Document and I hereby sign the CLA

@cholcombe973
Copy link

I also linked my email so that should be fixed as well

@rrichardson
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@kushudai kushudai self-requested a review June 26, 2023 22:25
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.

3 participants