Add a check for put operation with minimum one bin

[sahlone]

The pull request is for the interface to the put operation void put(WritePolicy policy, Key key, Bin... bins) throws AerospikeException The problem is the bins argument is a vararg and it's very easy to miss the argument and still pass the compilation phase. What I would like is void put(WritePolicy policy, Key key, firstBin Bin, Bin... bins) throws AerospikeException Which gives me type safety. When I tried to refactor it's a big change and thought, for now, the exception will still be helpful rather than get an error like AerospikeException: Error Code 4: Parameter error Which doesn't help at all.

I am not sure if there is a particular case where the Bin can be optional like in cases of overriding, but then again I realized what's the point of having a put operation without a data sent across

[BrianNichols]

Vararg parameters are used throughout the client and if it's checked in one place, it should be checked in all places. I don't think the overhead of these sanity checks provides any real value. These type errors are highly unlikely to occur in production and are eventually caught and logged by the server.

If you feel these checks are necessary, provide an interim method in your application code that performs extra validation before calling the AerospikeClient put() method.

[sahlone]

@BrianNichols You are right that the varargs is spread across the client and the important thing here is the major methods like put needs to be the first one to start. Talking about if it adds any value, The value it adds for me is that my mistakes will be caught in compile time rather than runtime(not in the case of the throw that I did now). ANd second I don't think anyone will go and check server everytime for the log about a request failure. Also look at the client error we have, does it convey any information ?. And most importantly we simply have a choice of making things better or not.

[BrianNichols]

Sometimes there is a trade-off between performance and ease of use. Aerospike tends to lean on the performance side of things. Extra validity checks can easily be added by extending AerospikeClient.

public class MyClient extends AerospikeClient {
    public MyClient(ClientPolicy policy, Host... hosts) {
        super(policy, hosts);
    }

    public void putBins(WritePolicy policy, Key key, Bin... bins) {
        if (bins.length < 1) {
            throw new AerospikeException("Require at least a single bin");
        }
        super.put(policy, key, bins);
    }
}

[sahlone]

In that case perhaps you can remove all the policy checks on client as well and return an encoded error code from server or just consider a default policy on server side like here

(policy == null){
   ///add default policy
}

[sahlone]

@BrianNichols I think this should help jvm uncommon trap , actually ths check will be optimized by the JIT

[Aloren]

Sometimes there is a trade-off between performance and ease of use. Aerospike tends to lean on the performance side of things.

In such case there is no tradeoff. Vararg in java is just a simple array and checking the size of array is o(1), meaning it's constant time, it's the same 'performance overhead' as accessing any other variable in the code. In comparison to the socket read/write operation this check is nothing, moreover it has more benefits than disadvantages:

1. programmers get more understandable errors and as a result it leads to faster investigation of any issues with the code.
2. client becomes smart enough not to send bad requests to server.
3. all consumers of the api are not obliged to extend api and add required validations by themselves.

[sahlone]

@Aloren Thank you for making it clear and helping with some understanding. @BrianNichols Is there any chance u can consider this ?