possible improvements

[petar-dambovaliev]

Hello, i've been trying out the client and i have some points that could possibly be improved.

I was using a "hello world" project that didn't have logging or anything like that setup. Therefore, i received the following error

Unable to communicate with server cluster: Failed to connect to host(s). The network connection(s) to cluster nodes may have timed out, or the cluster may be in a state of flux.

This error obviously doesn't say much. I had to go read the source code to see that some of the errors are not returned, but simply logged in debug! statements

2022-03-03T15:38:15.278Z DEBUG [aerospike::cluster::node_validator] Alias 127.0.0.1:3000 failed: Error(ServerError(InvalidCredential), State { next_error: None, backtrace: InternalBacktrace { backtrace: None } })

And the reason i encountered this error is because of the suboptimal design of the ClientPolicy data structure. It has the field pub user_password: Option<(String, String)>, public but will only work via the setter method set_user_password, because it is being hashed there.

Rust has a very powerful type system and we don't need to rely on documentation to suggest correct usage. We can enforce correct usage. I also don't see a reason of preemptively hashing the password. I would do it lazily, when the request is actually sent.

When i followed the tutorial and setup a EKS cluster, the default user was admin. However, as it seems, admin didn't have write permissions, for some reason. Which comes back to the fact that errors are not being returned and the client can't model his/her code based on them.

[petar-dambovaliev]

Macros for collections like as_list and as_map do not create the collections with the known length. This results in multiple unnecessary allocations.

[khaf]

Thanks Petar for your feedback, please keep them coming.

[petar-dambovaliev]

It would be nice if the UDFs were not just strings but went through a macro that enforced the aerospike rules for Lua. It would bring runtime errors into compile time.

[petar-dambovaliev]

Some functions' arguments have improper names, where bin: &str should actually be bin_name: &str or udf_name: &str where it should be module: &str or something like that. Naming is not super important but it can be confusing in the beginning, when you are trying to figure out how this client works.

For example

pub async fn execute_udf(
        &self,
        policy: &WritePolicy,
        key: &Key,
        udf_name: &str,
        function_name: &str,
        args: Option<&[Value]>,
    )

Inside the body of this function, udf_name is actually passed onto as package_name.

[petar-dambovaliev]

It would be nice if the UDFs were not just strings but went through a macro that enforced the aerospike rules for Lua. It would bring runtime errors into compile time.

To expand on this, the macro can also generate a rust function with the same name as the Lua function. So instead of calling it like

client
        .execute_udf(
            &WritePolicy::default(),
            &key,
            "map_merge",
            "map_merge",
            Some(&[as_val!(2)]),
        )
        .await;

you will call it like

map_merge(client, &WritePolicy::default(), as_val!(2)]).await

Which will push more errors into compile time, like incorrect arguments.

[petar-dambovaliev]

This enum was defined with the idea that there would be more than 1 language and possible additions in the future. However, with this definition, adding another variant is a breaking change. It is not clear how big of a problem that is, but still. Adding #[non_exhaustive] would be good.

/// User-defined function (UDF) language
#[derive(Debug)]
pub enum UDFLang {
    /// Lua embedded programming language.
    Lua,
}