app/vlselect: expose the VL-Partial-Response HTTP header

[vadimalekseev]

The header value can be true or false, depending on whether the query was executed partially or completely. For streaming queries that do not require buffering the final result in memory, this header will contain the value unknown.

Closes #718
Updates #1208

[cubic-dev-ai]

1 issue found across 5 files

<sub>Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic</sub>

[func25]

This may hide non-availability errors from the user when at least one vlstorage node succeeds, e.g.,[nil, nonAvailabilityError] returns no the first nil element here.

[vadimalekseev]

Did I understand correctly that this issue also exists in the master branch?

[func25]

I was under the impression that the current behavior was correct, but it turns out this logic was correct before and was accidentally regressed by 348f103. So this bug already exists on master

[func25]

This read may race with atomic writes to qs.QueryCompleteness. Adding a method such as QueryStats.GetQueryCompleteness(), which uses atomic.LoadUint32(), and use it here?

[vadimalekseev]

Initially, there was such a method, but I removed it to simplify the code, because:

1. Only one goroutine can call writeResponseHeaders.
2. The other goroutines wait for the first one to finish calling writeResponseHeaders here:
   

   VictoriaLogs/app/vlstorage/netselect/netselect.go

   Line 199 in 3622e0a

   processBlock(&db)

   before updating the QueryCompleteness value here:

   VictoriaLogs/app/vlstorage/netselect/netselect.go

   Line 136 in 3622e0a

   defer qctx.QueryStats.UpdateAtomic(qsLocal)

Should we explicitly sync this, do you think?

[vadimalekseev]

Yeah, I double-checked the code, and the fact that it doesn't need synchronization is really counter-intuitive. I will add explicit synchronization

See the comment bellow.

[vadimalekseev]

Even though this counter-intuitive behavior is basically the core of the whole PR: we cant let another goroutine write QueryCompletenessComplete, otherwise we'll set the partial response to 'false' for a streaming request.

[vadimalekseev]

More explicit (but complex) implementation described here: #718 (comment)

[func25]

Sorry I don't understand, I don't mean waiting until all goroutines finish before writing the header. I mean using atomic load for reading QueryCompleteness, since the field is updated via atomic/CAS elsewhere. This doesn't change the timing. Could you explain more?

[vadimalekseev]

Hey @func25, I've updated the runQuery function, so QueryStats updates only after the wg.Done(). Could you please check again?

Previously, there actually could have been a race condition if an empty response was received from vlstorage. In that case, we would never have called writeBlock, which used to act as a mutex due to writeResponseHeadersOnce.

This could lead to a situation where one vlstorage returned an empty response, while the second one was unavailable. In this case, we could potentially set QueryCompletenessComplete instead of Partial. The current code should avoid this situation.

[func25]

If you fix the issue in master, I think this snippet is not needed, it can be replaced with

var unavailableErr error
successes := 0

for _, err := range errs {
	if err == nil {
		successes++
		continue
	}
        if !isUnavailableBackendError(err) {
	        ...
        }
        unavailableErr = err
}