Some fixes to Aerospike Client Library

[CMoore-Darwinium]

Hi Aerospike Team!

I'm Caleb. I might have met you or your colleagues when I worked for ThreatMetrix. They were always a very happy Aerospike customer.

We've been using your Rust Client Lib for a while. We've encountered a few rough edges but was wanting to contribute back some fixes.

These patches were made for our own internal use, but I'm sending it here mostly to start a discussion on how we can collaborate in the future. It contains is a fix to two unrelated issues.

Firstly, the ListReturnType was forced into an Enum (and lost the capability to use the INVERTED flag alongside other flags). After that, it was then cast to a u8, meaning it lost the INVERTED flag completely, even if you passed that and nothing else. My change involves a breaking change to the interface to take a u64 like the C version and allow it to be ORed together with other flags, but I'm not sure if this will be acceptable (I can change it to take a separate parameters?).

Secondly I went over the Msgpack encoding and made the rust encoding the same as the C encoding code use in C/AQL/Python library. This has a particular impact on the choice between signed and unsigned expression, now like the C API it picks signed integers only for negative numbers, this makes a huge difference in the Rust implementation that distinguishes between Uint and Int values on the way out, by being consistent, it makes it less likely to break when something is modified by python.

Please, let me know what you think.

-Caleb

[khaf]

Hi Caleb, thanks for your PR. I had a cursory look, but as you said, the breaking change requires a bit more scrutiny than usual so I'll need a bit more time to think about other workarounds in the ecosystem that people use for these situations. Off the bat, I'm more against having just u64 values and losing Rust's sophisticated type system advantage. Let me think about it and I'll get back to you in a few days.

[CMoore-Darwinium]

Hi Khosrow,

I was thinking it would be nice to implement a trait that is implemented for both enums (for instance ListReturnType) and for T: ToIterator, this would mean you could pass in an enum directly, an array of enums, an Option, a vector, etc to the function. It would be a non-breaking change in that case.

If this is acceptable to you, I'll implement it for ListReturnType and possibly some other flags when I have time.

-Caleb

[jonas32]

I also had this change in mind for some time. Also replacing String with ToString at some places would be great. In general having more trait based interaction could be useful. For example Bins and Value could be a lot easier to work with when there is a trait for ToValue or ToBins in place. Just to mention this in advance: Generics sometimes are problematic with async. I also had to remove this at a multiple places for the async port.

[khaf]

@CMoore-Darwinium I have nothing against changing to traits for input params, provided they don't introduce unintentional side effects. The devil is in the details though, so I'd invite a PoC PR to discuss said details. @jonas32 Thanks for your input. I'm surprised, what's the relationship with the async? How can these concepts be related?

[jonas32]

Using Traits brings in some complexity with thread safety. Its not always that easy to resolve that without other trade offs. In general its not an unsolvable problem to use Traits in that regard, but its additional overhead.

[CMoore-Darwinium]

@khaf Hi Khosrow,

I've had a think and done a few changes to use traits in order to make it source-compatible with and without the modifications.

For instance it now accepts something like:

aerospike::operations::exp::write_exp(BIN, &filter_expression, aerospike::operations::exp::ExpWriteFlags::CreateOnly),

as well as:

aerospike::operations::exp::write_exp(BIN, &filter_expression, [aerospike::operations::exp::ExpWriteFlags::CreateOnly, aerospike::operations::exp::ExpWriteFlags::PolicyNoFail]),

I've also followed this pattern for ListWriteFlags.

For ListReturnType I did something slightly different. Since only Inverted is a flag and the others are a conventional enum, I've created am InvertedListReturn struct that can take an enum inside it and be used in its place.

aerospike::operations::lists::remove_by_rank_range(
    BIN,
    -range,
    aerospike::operations::lists::ListReturnType::Count,
),

Can be inverted as so:

aerospike::operations::lists::remove_by_rank_range(
    BIN,
    -range,
    aerospike::operations::lists::InvertedListReturn(aerospike::operations::lists::ListReturnType::Count),
),

This pattern will not try to combine together various mutually exclusive flags as it would if it used the pattern used by ExpWriteFlags and ListWriteFlags above.

Hi @jonas32 , Regarding traits. In this changeset, these traits are only used for creating operations and policies This sort of stuff is presumably only ever going to be synchronous as it's not really accessing resources or doing any real work, it's just initializing a few simple structures.

[jonas32]

I think its a good Idea. For this high level interfaces it will be a great feature. As i said, its only a "problem" for the async parts deep down in the client and also solvable there. I can think of many places where this would simplify the client.

[khaf]

@CMoore-Darwinium This looks cool! I'm dealing with release work for another client right now. Give me a couple of weeks and I'll be back on Rust again.

[CMoore-Darwinium]

Hi @khaf , I was wondering if there is any update here?

[khaf]

Hi @CMoore-Darwinium , what's the status on this PR? Do you think you can rebase it on top of the async branch so I can review your changes specifically?

[CMoore-Darwinium]

Hi @CMoore-Darwinium , what's the status on this PR? Do you think you can rebase it on top of the async branch so I can review your changes specifically?

Sure. I'll try to do it in the next week or so.

[khaf]

@CMoore-Darwinium Did you manage to rebase your branch it on top of the async branch?

[CMoore-Darwinium]

Oh wow... I said I'd do that in feburary.

Sorry, I work in a startup and we've been a little bit crazy. I'll try it this evening.

I've renamed and rebased my patch, there is a new PR