Re: juju/retry take 2 - looping

[roger peppe]

On 26 October 2016 at 04:46, John Meinel  wrote:
>> I think this is a hint that this is the wrong approach. The edge-cases
>> begin showing the cracks in the abstraction and end up making the code more
>> complex. Consider your example instead of:
>>
>> var finalResult Foo
>> for loop := retry.Loop(spec); loop.Next(); {
>> result, err := SomeFunc(blah)
>> if err != nil || resultNotGoodEnough(result) {
>> continue
>> }
>>
>> finalResult = result
>> break
>> }
>>
>> There are no special errors, no mixing of concerns, just a boring
>> imperative loop. It works like any other loop written in Go.
>
>
> The one things I'll specifically note about your "simple" example is that it
> doesn't actually handle errors. The fact that the "quick way to write it" is
> actually wrong is the specific argument of this thread.
> In your loop you have a way to generate a FinalResult, but no handling of
> the actual "I couldn't get a result". The way above that you can tell the
> loop failed would be to check if FinalResult doesn't have a valid value.
>
> This, at least, is the point I think Tim is bringing up of "hard to get
> wrong".

At least that's obvious from looking at the code, which is just normal
Go code like you might find anywhere. The looping construct doesn't
have anything to do with the error values that the code is dealing with.

In Tim's package, we've got *two* places that are storing the error,
inside the iterator and outside it. You can't always rely on the
error that's inside the iterator because you might not have passed it in.

Take this earlier example:

var result Foo
var err error
for loop := retry.Loop(spec); loop.Next(err); {
 result, err = SomeFunc(blah)
 if isFatalErr(err) {
 break
 }
}
if err != nil {
   // what ever
}
// continue using result

In this case we sometimes want to use err (if the loop
hasn't run to completion) and sometimes loop.Error
(if it has), but in the latter case we probably want to
use err too because loop.Error returns the last-but-one
error not the most recent error.

And in this example, which I think Katherine adapted
for demo purposes, it's still not handling the error
in exactly the same way as Katherine's, so
it doesn't seem like it's "harder to get wrong" really:

var result Foo
var err error
for loop := retry.Loop(spec); loop.Next(err); {
   result, err = SomeFunc(blah)
   if err == nil && resultNotGoodEnough(result) {
 err = retry.ErrTryAgain
   }
}
if err != nil {
   // what ever
}
// continue using result

Also, that example isn't fit for purpose because it
discards the error, so the code can't return what the most
recent issue was when the loop times out.

FWIW having evaluated the options, the Snappy team have decided to go
with the package I proposed. I landed it in gopkg.in/retry.v1.  Feel free
to use it if you think you can live with it.

  cheers,
rog.

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[John Meinel]

...


>
> I think this is a hint that this is the wrong approach. The edge-cases
> begin showing the cracks in the abstraction and end up making the code more
> complex. Consider your example instead of:
>
> var finalResult Foo
> for loop := retry.Loop(spec); loop.Next(); {
> result, err := SomeFunc(blah)
> if err != nil || resultNotGoodEnough(result) {
> continue
> }
>
> finalResult = result
> break
> }
>
> There are no special errors, no mixing of concerns, just a boring
> imperative loop. It works like any other loop written in Go.
>

The one things I'll specifically note about your "simple" example is that
it doesn't actually handle errors. The fact that the "quick way to write
it" is actually wrong is the specific argument of this thread.
In your loop you have a way to generate a FinalResult, but no handling of
the actual "I couldn't get a result". The way above that you can tell the
loop failed would be to check if FinalResult doesn't have a valid value.

This, at least, is the point I think Tim is bringing up of "hard to get
wrong".

John
=:->
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[Katherine Cox-Buday]

Tim Penhey  writes:

> Forcing continue/break does not make it like all boring imperative
> loops.

I would like to understand this better.

Here's why I think they're boring:
- They're Go keywords.
- They're present in many other languages and are well understood.
- I see breaks/continues all the time in loops.

In my opinion, it's much easier to understand and support a loop that makes use 
of these keywords than it is to invent a new way to break out of a loop.

Can you elaborate on why do you think these keywords are worse than checking 
for a special error instance?

-- 
Katherine

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[roger peppe]

On 25 October 2016 at 22:40, Tim Penhey  wrote:
> On 26/10/16 10:30, Katherine Cox-Buday wrote:
>>
>> I think this is a hint that this is the wrong approach. The edge-cases
>> begin showing the cracks in the abstraction and end up making the code more
>> complex. Consider your example instead of:
>>
>> var finalResult Foo
>> for loop := retry.Loop(spec); loop.Next(); {
>> result, err := SomeFunc(blah)
>> if err != nil || resultNotGoodEnough(result) {
>> continue
>> }
>>
>> finalResult = result
>> break
>> }
>>
>> There are no special errors, no mixing of concerns, just a boring
>> imperative loop. It works like any other loop written in Go.
>
>
> I was trying to reduce the need for continue/break. Forcing continue/break
> does not make it like all boring imperative loops.
>
> The err used inside the loop should be declared outside if you want to be
> able to see if the loop succeeded or failed.

Honestly, it's just a loop that tries *something* several times with delays.
It's not always because of something that looks like an error. Trying to make it
look like an error makes the primitive less easy to understand
and more considerably more difficult to use in some cases
(consider retrying because of a particular HTTP response code).

  rog.

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[Tim Penhey]

On 26/10/16 10:30, Katherine Cox-Buday wrote:

I think this is a hint that this is the wrong approach. The edge-cases begin 
showing the cracks in the abstraction and end up making the code more complex. 
Consider your example instead of:

var finalResult Foo
for loop := retry.Loop(spec); loop.Next(); {
result, err := SomeFunc(blah)
if err != nil || resultNotGoodEnough(result) {
continue
}

finalResult = result
break
}

There are no special errors, no mixing of concerns, just a boring imperative 
loop. It works like any other loop written in Go.


I was trying to reduce the need for continue/break. Forcing 
continue/break does not make it like all boring imperative loops.


The err used inside the loop should be declared outside if you want to 
be able to see if the loop succeeded or failed.


Tim

--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[Katherine Cox-Buday]

Tim Penhey  writes:

> The rationale behind that is to make the the library handle the 80%
> case easily. The main reason of passing the error into Next is so you
> don't have to do the check and explicit break within the loop.

I will argue here that this goes against a core principle of Go which is to be 
informed and care about your error path.

> Sure, but this could be trivially handled by assigning something to
> err (consider if we made a public error in the retry package for
> this):
>
> var result Foo
> var err error
> for loop := retry.Loop(spec); loop.Next(err); {
>result, err = SomeFunc(blah)
>if err == nil && resultNotGoodEnough(result) {
>  err = retry.ErrTryAgain
>}
> }
> if err != nil {
>// what ever
> }
> // continue using result

I think this is a hint that this is the wrong approach. The edge-cases begin 
showing the cracks in the abstraction and end up making the code more complex. 
Consider your example instead of:

var finalResult Foo
for loop := retry.Loop(spec); loop.Next(); {
result, err := SomeFunc(blah)
if err != nil || resultNotGoodEnough(result) {
continue
}

finalResult = result
break
}

There are no special errors, no mixing of concerns, just a boring imperative 
loop. It works like any other loop written in Go.

-- 
Katherine

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[Tim Penhey]

Some comments first, then addressing issues:

I thought it might help if I walked you through my thought process of 
making this change.


* the purpose of the retry package is to provide a way to retry actions 
that may fail until they succeed, or some predetermined limit is hit

* the way that failure is idiomatically handled is with an error
* I wanted the interface to be very simple to use, and hard to get wrong
* it should work inline in a simple for loop
* the library should make the simple case very simple

var result Foo
var err error
for loop := retry.Loop(spec); loop.Next(err); {
 result, err = SomeFunc(blah)
}
if err != nil {
 // what ever
}
// continue using result

What about fatal errors?

var result Foo
var err error
for loop := retry.Loop(spec); loop.Next(err); {
 result, err = SomeFunc(blah)
 if isFatalErr(err) {
 break
 }
}
if err != nil {
   // what ever
}
// continue using result


On 21/10/16 06:33, roger peppe wrote:

On 20 October 2016 at 16:30, Katherine Cox-Buday
 wrote:

John Meinel  writes:


As commented on the pull request, passing 'err' into Next() initially feels 
weird, but
it allows a big improvement to whether the loop exited because we ran out of
retries, or it exited because the request succeeded. (loop.Error()
tells us either way.)


I don't understand why the retry logic has the concern of why the loop exits 
(i.e. Next(error)).


The rationale behind that is to make the the library handle the 80% case 
easily. The main reason of passing the error into Next is so you don't 
have to do the check and explicit break within the loop.



The motivation of moving towards loops was so that the concern of what's being 
retried is brought back to be inline with the surrounding code. Having the 
retry mechanism inspecting errors doesn't fall in line with that goal.

In my mind, any retry solutions only needs to cover two things:
1. Provide a primitive that will delay a for iteration in a controlled way.
2. Be preemptable.

Everything else is the logic of the thing you're retrying, including why it 
eventually succeeded/failed.


Here is were we disagree slightly. I also think that very high on the 
list are:

   * easy to get right
   * hard to get wrong


That's well put, and fits well with my thoughts too, thanks.

See also Pawel Stolowski's comment on
https://github.com/juju/retry/pull/5. He's from the Snappy
team who have also been trying to move towards using a standard retry mechanism.
There are many possible reasons for wanting to retry - error values
are just one possibility.


Sure, but this could be trivially handled by assigning something to err 
(consider if we made a public error in the retry package for this):


var result Foo
var err error
for loop := retry.Loop(spec); loop.Next(err); {
   result, err = SomeFunc(blah)
   if err == nil && resultNotGoodEnough(result) {
 err = retry.ErrTryAgain
   }
}
if err != nil {
   // what ever
}
// continue using result

Tim

--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[roger peppe]

On 20 October 2016 at 16:30, Katherine Cox-Buday
 wrote:
> John Meinel  writes:
>
>> As commented on the pull request, passing 'err' into Next() initially feels 
>> weird, but
>> it allows a big improvement to whether the loop exited because we ran out of
>> retries, or it exited because the request succeeded. (loop.Error()
>> tells us either way.)
>
> I don't understand why the retry logic has the concern of why the loop exits 
> (i.e. Next(error)).
>
> The motivation of moving towards loops was so that the concern of what's 
> being retried is brought back to be inline with the surrounding code. Having 
> the retry mechanism inspecting errors doesn't fall in line with that goal.
>
> In my mind, any retry solutions only needs to cover two things:
> 1. Provide a primitive that will delay a for iteration in a controlled way.
> 2. Be preemptable.
>
> Everything else is the logic of the thing you're retrying, including why it 
> eventually succeeded/failed.

That's well put, and fits well with my thoughts too, thanks.

See also Pawel Stolowski's comment on
https://github.com/juju/retry/pull/5. He's from the Snappy
team who have also been trying to move towards using a standard retry mechanism.
There are many possible reasons for wanting to retry - error values
are just one possibility.

Another thing I'd like to mention is that my retry package suggestion
takes care to keep
loop strategy (the parameters that govern how we intend to retry)
separate from the loop
runtime requirements (the clock and stop channel). This means that
it's perfectly
reasonable to store a loop strategy as a "constant" global variable,
as we do with
testing.LongAttempt for example, and validate the parameters just once at init
time rather than every time an attempt is started.

On a slightly more technical note, the backoff approach used in
github.com/juju/retry
means that it's not possible to have an attempt that adjusts the delays based on
the actual length of time that an attempt took (for example, if an
attempt took 2s
but the backoff function specifies that the delay should be 1s, it may
be appropriate
to wait for 0s or 1s before starting the next attempt). That wouldn't
be hard to fix though.

The lack of a More (HasNext as was) method in the proposed package
also makes the
loop idiom less convenient. If I'm looping trying to acquire a value,
it's easier to
declare the value and the error inside the loop and return the value on success
rather than assigning it outside.

For example, I think it's arguably nicer to do:

for a := strategy.Start(); a.Next(); {
  someVal, err := something()
  if err == nil || !shouldRetry(err) {
   return someVal, err
  }
  if !a.More() {
   return nil, errors.Annotatef(err, "failed after too
many attempts")
  }
  }

than:

var err error
loop := retry.Loop(spec)
for loop.Next(err) {
var someVal somepkg.Something
someVal, err = something()
if err == nil || !shouldRetry(err) {
 return someVal, err
}
 }
 return nil, err

In the second example, we already need to check the error value, so
why should we need to pass it to loop.Next?

  cheers,
rog.

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[Katherine Cox-Buday]

John Meinel  writes:

> As commented on the pull request, passing 'err' into Next() initially feels 
> weird, but
> it allows a big improvement to whether the loop exited because we ran out of
> retries, or it exited because the request succeeded. (loop.Error()
> tells us either way.)

I don't understand why the retry logic has the concern of why the loop exits 
(i.e. Next(error)).

The motivation of moving towards loops was so that the concern of what's being 
retried is brought back to be inline with the surrounding code. Having the 
retry mechanism inspecting errors doesn't fall in line with that goal.

In my mind, any retry solutions only needs to cover two things:
1. Provide a primitive that will delay a for iteration in a controlled way.
2. Be preemptable.

Everything else is the logic of the thing you're retrying, including why it 
eventually succeeded/failed.

-- 
Katherine

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[Ian Booth]

I really like where the enhancements are headed. I feel they offer the syntax
that some folks wanted, with the safety and validation of the initial
implementation. Best of both worlds.

On 20/10/16 13:09, Tim Penhey wrote:
> Hi folks,
> 
> https://github.com/juju/retry/pull/5/files
> 
> As often is the case, the pure solution is not always the best. What seemed
> initially like the best approach didn't end up that way.
> 
> Both Katherine and Roger had other retry proposals that got me thinking about
> changes to the juju/retry package. The stale-mate from the tech board made me
> want to try another approach that I thought about while walking the dog today.
> 
> I wanted the security and fall-back of validation of the various looping
> attributes, while making the call site much more obvious.
> The pull request has the result of this attempt.
> 
> It is by no means perfect, but an improvement I think. I was able to trivially
> reimplement retry.Call with the retry.Loop concept with no test changes.
> 
> The tests are probably the best way to look at the usage.
> 
> Tim
> 

-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

Re: juju/retry take 2 - looping

[John Meinel]

As commented on the pull request, passing 'err' into Next() initially feels
weird, but it allows a big improvement to whether the loop exited because
we ran out of retries, or it exited because the request succeeded.
(loop.Error() tells us either way.)

I wonder if we want to push harder on always having a Stop channel, as
*production* code should probably always have a way to stop early.

John
=:->

On Thu, Oct 20, 2016 at 7:09 AM, Tim Penhey 
wrote:

> Hi folks,
>
> https://github.com/juju/retry/pull/5/files
>
> As often is the case, the pure solution is not always the best. What
> seemed initially like the best approach didn't end up that way.
>
> Both Katherine and Roger had other retry proposals that got me thinking
> about changes to the juju/retry package. The stale-mate from the tech board
> made me want to try another approach that I thought about while walking the
> dog today.
>
> I wanted the security and fall-back of validation of the various looping
> attributes, while making the call site much more obvious.
> The pull request has the result of this attempt.
>
> It is by no means perfect, but an improvement I think. I was able to
> trivially reimplement retry.Call with the retry.Loop concept with no test
> changes.
>
> The tests are probably the best way to look at the usage.
>
> Tim
>
> --
> Juju-dev mailing list
> Juju-dev@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailm
> an/listinfo/juju-dev
>
-- 
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev

juju/retry take 2 - looping

[Tim Penhey]

Hi folks,

https://github.com/juju/retry/pull/5/files

As often is the case, the pure solution is not always the best. What 
seemed initially like the best approach didn't end up that way.


Both Katherine and Roger had other retry proposals that got me thinking 
about changes to the juju/retry package. The stale-mate from the tech 
board made me want to try another approach that I thought about while 
walking the dog today.


I wanted the security and fall-back of validation of the various looping 
attributes, while making the call site much more obvious.

The pull request has the result of this attempt.

It is by no means perfect, but an improvement I think. I was able to 
trivially reimplement retry.Call with the retry.Loop concept with no 
test changes.


The tests are probably the best way to look at the usage.

Tim

--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/juju-dev