Unblock relaxed write consistency level

[takayuki]

This pull request contains two commits; one introducing an option in cmd/influx to select write consistency level, which has been set to 'any' by default, and another fixing writes possibly blocked even with relaxed write consistency level (e.g. any, one, or quorum).

Assume you have a 3-node cluster and one of those nodes went down. Write requests with write consistency level more relaxed than, or equal to quorum, should be completed immediately and successfully. But, the requests are possibly blocked waiting the dead node to respond.

[beckettsean]

@corylanou

[corylanou]

Why did this change? I don't see us using i, but I could be missing it. The original is much clearer and idiomatic in Go.

If it was to stop using the nodeID, you can simply do:

for range shard.OwnerIDS {

[jwilder]

Agreed. The AsyncWriteResult and for loop change is unnecessary.

[corylanou]

Overall, looks good. I would like to see a test or two added. One for testing the new flag added and that it parses correctly. Would like one more set of eyes as well to the change on the cluster points writer. Perhaps @otoolep or @jwilder

[otoolep]

Thanks @takayuki

@jwilder definitely needs to see this. I understand the change to the write path code, but @jwilder needs to confirm that it is the right thing to do.

[jwilder]

Moving the write >= required to the upper loop is the correct fix. I was able to verify that a slow writer could cause a write of any, one or quorum to block and return a timeout even after it successfully wrote to the required nodes. If the node was down (an we could not connect), it would return as quickly as possible in the current code.

[jwilder]

@takayuki If you could rebase, revert the looping change, and make sure you've signed the CLA. This change should be good to go. Thanks for the fix!

[takayuki]

Thank you @jwilder Would you spend some more with me, letting me think about the concern I originally had?

There are two loops involved here, one for starting goroutines and another for waiting for them to complete their tasks. The problem is, I thought, the second loop might be getting the results in a different order from one in which goroutines started. That's why I had to add AsyncWriteResult handling out-of-order arrivals.

Here is a conceptual example, taking the suggestion from @corylanou; the first loop starts node#1, then node#2, whereas the second loop is going to get the results from node#2, then node#1. I thought PointsWriter would have to take it into account.

    ownerIDs := [2]int{1,2}
    ch := make(chan int, len(ownerIDs))
    for _, nodeID := range ownerIDs {
        go func(nodeID int) {
            // node#1 sleeps for 10 secs, node#2 for 5 secs,
            // so node#2 is going to finish earlier.
            time.Sleep(time.Duration(10/nodeID)*time.Second)
            ch <- nodeID
        }(nodeID)
    }
    for range ownerIDs {
        select {
        case nodeID := <-ch:
            // node#2 arrives earlier, then node#1 later.
            fmt.Println(nodeID)
        }
    }

[otoolep]

Yeah, that's what I thought too @takayuki -- the new loop made sense to me.

[otoolep]

I've confirmed @takayuki has signed the CLA.

[jwilder]

@takayuki @otoolep The order in which the response arrive does not matter from a write perspective. This code is writing to all shard owners in parallel and returning as soon as it receives the required number of successful responses.

It does look like there is a bug in the logging here though: https://github.com/influxdb/influxdb/pull/3901/files#diff-99b560e650ccc1fa3f20c23df5a9b41bL287 which might be what the looping change was intended to fix. It could log the wrong NodeID if the results arrive out of order. That is not right so I think your looping change makes sense in light of that issue.

[jwilder]

Thanks @takayuki !

[otoolep]

Yes, your second point is the one @takayuki and I were making, it was just a logging thing.