No goprocess

[gammazero]

• remove goprocess from api
• replace goprocess with context.Context
• replace ResultsWithProcess with ResultsWithContext

[hsanjuan]

I am a bit confused about the following:

• We have ResultsWithContext... but cancelling that context does not cancel the thing. It seems that context is just to pass on the proc and maybe do tracing etc.
• Instead we have an internal context in new results builder, which gets cancelled on Close(), which then closes a channel, which then is returned on Done().

Couldn't we have a NewResultsBuilder(context.Context, Query) and Results.results on context cancellation and we don't need internal context nor additional channels inside?

Or, perhaps leave NewResultsBuilder(Query) as is, and add a ResultsBuilder.TriggerQuery(context.Context) <-chan Result ? (and remove Next()). This is if we are to comply with "pass no context on New()".

I am not sure how much code out there is making use of Result's NextSync() and Rest(), but when I read this it begs the question of why we are doing basic channel management for the user, like doing reads-blocking, or reading all results from a channel and putting them on a slice, or closing and managing cancellations in ways that the user could do very easily in their code. It seems the main thing is to be able to make a query and return a channel to read results and everything else could potentially go away.

[hsanjuan]

Do we really need a closed channel, or would it essentially correspond to ctx.Done() ?

[gammazero]

This channel is needed in addition to the ResultBuilder.ctx. Cancelling the internal context (when Results is closed) tells the goroutine handling processing that it needs to exit and close the results (output) channel afterwards. -- That is the purpose of ctx. The purpose of closed is to signal that the goroutine has exited and no more results are being read by the goroutine.

In other words...

• internal ctx signals "closing"
• closed, and therefore Results.Done, signals finished closing.

This is actual critical for some uses of query results when it is necessary to wait for the goroutine, started by ResultsWithContext to exit before doing something with items that were closed over by the function passes to ResultsWithContext.

[hsanjuan]

Good chance to get rid of this method...

[gammazero]

I could get rid of it... or leave it now that it is technically fixed because callers can now select over Results.Done when writing to the results input channel.

[hsanjuan]

it begs the question of why we are doing basic channel management for the user

Iirc there are some helper methods doing things like potentially sorting results. But are these used anywhere relevant?

[gammazero]

it begs the question of why we are doing basic channel management for the user

This was done to preserve the functionality that the goprocess code offered, and therefore minimize structural changes to calling code. The ResultsWithContext, ResultsWithChan, ResultsBuilder. and friends are there to help Query implementors create query result generators. These generators need to have some common functionality like being cancelable by the query client. This tries to do the channel management that most implementors would otherwise have to do some form of.

The client need to give the Query implementor: 1. a way to get the result, 2. a place to write the result, 3. a means to receive a cancel signal. The Query implementor needs to give the client a signal when it is no getting/writing any more results.

I am thinking that ResultBuilder can go away.

Iirc there are some helper methods doing things like potentially sorting results. But are these used anywhere relevant?

They are used in a few places, mostly do to result filtering for datastores that do not provide that functionality natively.

[hsanjuan]

The Query implementor needs to give the client a signal when it is no getting/writing any more results.

If you are getting the query results from a channel, the signal would be when that channel is closed, no need for a separate signal (implies that we remove other methods of getting the results).

Anyways, I understand keeping functionality without going into full refactor mode is best for now.

[hsanjuan]

Minor nit unless I'm missing something.

[gammazero]

@hsanjuan I made what is hopefully a final change, which is to get rid of ResultBuilder. This allowed the query API to be greatly simplified as well as made some of the less obvious logic (discussed above) totally unnecessary. Please have one more look at this PR to double check.

Also, you can see how this change simplifies users of go-datastore:

• Use go-datastore without go-process go-ds-crdt#267