Missing context.Context in all API methods

[rantav]

In golang it is considered best practice that outgoing requests would accept a context.Context https://golang.org/pkg/context/

The motivation being that in many cases it is desirable to either cancel a request or timeout (deadline) a request.
Although there is a timeout property on aerospike's client itself, this timeout is global to all operations while in reality you sometimes want a different timeout for different calls.

Ideally I would modify, for example:

Delete(policy *as.WritePolicy, key *as.Key) (bool, error)

to:

Delete(ctx context.Context, policy *as.WritePolicy, key *as.Key) (bool, error)

This change would apply to all client API methods.

What are the pros or cons of making this change? Other than the obvious fact that this is an API change that requires code changes from library users?

Right now I'm left with the option of wrapping the client myself with such context, which is a burden I would prefer the core lib to maintain.

Example:

func (c ctxClient) Delete(
	ctx context.Context,
	policy *as.WritePolicy,
	key *as.Key,
) (deleted bool, err error) {
	const opName = "Delete"

	finished := make(chan bool, 1)
	go func() {
		deleted, err = c.rawClient.Delete(policy, key)
		finished <- true
	}()

	select {
	case <-finished:
	case <-ctx.Done():
		err = errors.Errorf("context done (%s) with operation %s", ctx.Err(), opName)
	}
	return
}

This topic had been discussed before in a narrow context (no pun intended) but I want to reopen the discussion in a wider scope #218

Thank you

[khaf]

Although there is a timeout property on aerospike's client itself, this timeout is global to all operations while in reality you sometimes want a different timeout for different calls.`

This statement is clearly wrong, since the timeout on the policy (in this case a WritePolicy) works only for the call you have passed it to.

context.Context came out a year after we released the Go client, and since there is a lot of existing code that depends on the current API, changing all the API signatures is a no go at this stage.

You are also proliferating your goroutines at an alarming rate with your implementation, which may add significant latency under high load.

[rantav]

This statement is clearly wrong, since the timeout on the policy (in this case a WritePolicy) works only for the call you have passed it to.

This is indeed something I missed (actually was looking for it). Good to know.

context.Context came out a year after we released the Go client, and since there is a lot of existing code that depends on the current API, changing all the API signatures is a no go at this stage.

Understood. But since it's becoming a standard in many Golang libs I would consider it for a future version.

You are also proliferating your goroutines at an alarming rate with your implementation, which may add significant latency under high load.

Indeed, that is the biggest cons. So I am left with the tradeoff of either supporting context as well behaved libraries should and by that risk goroutine proliferation, or just use the BasePolicy Timeout (thanks for the tip) and not support a context.

[ItalyPaleAle]

We would really like this feature for Dapr too

[khaf]

The Aerospike Go Client is a high performance library that supports hundreds of thousands of transactions per second per instance. Context support would require us to spawn a new goroutine for every request, adding significant overhead to the scheduler and GC.
I am convinced that most users would benchmark their code with the context support and decide against using it after noticing the incurred penalties.
I'm willing to consider adding separate API with Context support, though I'd need concrete user demand for it. Maybe in the form of Up/Down votes on this comment.