Closing circuit when majority of requests are bad requests

[kollektiv]

Our service has a client that does 90%+ bad requests (errors that satisfy the BadRequest interface); we wrap DynamoDB ConditionalCheckFailedException errors from aws-sdk-go in SimpleBadRequest. Occasionally it will switch from a mode of doing bad requests, to a mode of making successful requests that timeout, triggering the circuit to open, and then go back to doing bad requests. Since bad requests are not considered for closing the circuit, the circuit remains open until the hystrix closer happens to allow a non-bad request through.

What are your thoughts on allowing bad requests to be considered for closing the circuit?

[cep21]

I'm travelling to taiwan for 9 days. How do you propose doing this in a backwards compatible ish way?

[kollektiv]

In checkErrBadRequest, what if it checks the error for interface { SuccessfulResponse() bool }, and if SuccessfulResponse() returns true, then force close the circuit?

[cep21]

You want a successful error? Hmm that seems suspect. I think you want to return a tuple of ((response, error), error) instead since it sounds like you have two classes of errors, circuit and dynamodb. I can give more thoughts in 10 days but am in taiwan atm

[cep21]

A few thoughts.

1) Are you sure you want a ConditionalCheckFailedException to force close your circuit? Force close seems severe.

2) I think you want something like this (In serious pseudocode). The reason is that you want to eventually get back to the caller error objects that should be considered successful for the circuit. That's a bit of a strange perspective from the circuit's point of view, so you'll wan to wrap the return.

type Tuple struct {
  Result interface{}
  Err error
}

func DoAction(ctx context.Context) (result, error) {
  tup, err := circuit.Execute(CircuitFunc)
  if err != nil { return nil, error}
  return tup.Result, tup.Err
}

func CircuitFunc(ctx context.Context) (interface{}, error) {
  res, err := dynamodb.execute(ctx)
  if err.(ConditionalCheckFailedException) {
     return Tuple {Result: nil, Err, err}, nil
  }
  if err != nil {
    return nil, err
  }
  return Tuple{Result: res}, nil
}

Let me know if this will work.

[kollektiv]

I think I want ConditionalCheckFailedException to count as a successful response to end up closing the circuit. Force closing does seem severe.

I think 2. will work. Our application is using circuitgen to encapsulate calling circuits, so I would want to generalize the wrapping and unwrapping

[kollektiv]

For 2, here is how circuitgen could handle it https://github.com/twitchtv/circuitgen/pull/5

// Example ShouldSkipError implementation for the DDB client
ShouldSkipError: func(err error) bool {
    aerr, ok := err.(awserr.Error)
    return ok && aerr.Code() == dynamodb.ErrCodeConditionalCheckFailedException
}

[cep21]

Is it ok to close this ticket?