add renewal for scanner to prevent lease timeout

[tanay-vakharia]

This change adds an option to Scan (Renew) that allows for renewal of scanners during periods of starvation to prevent lease timeout. Currently, we are renewing every lease timeout / 2 seconds (10s currently).

[cluckett-arista]

I don't normally work on this, so just a few general nits and a request to run gofmt

[aaronbee]

A different way to do this would be to start a goroutine that sleeps on a ticker and context.

renewCtx, cancel := context.WithCancel(s.Context())
s.renewCancel = cancel
go s.renewLoop(renewCtx)

And renewLoop would look like:

func (s *scanner) renewLoop() {
    t := time.Ticker(s.rpc.RenewInterval()
    defer t.Stop()
    for {
        select {
        case <-t.C:
             if err := s.renew(); err != nil {
                    return
              }
        case <-ctx.Done():
            return
        }
    }
}

The advantage here is that the renewLoop routine will clean itself up immediately after the context is canceled and doesn't have to wait for the timer to go off to notice the context is done. And I think it's slightly simpler to have a loop rather than renewScanner() needing to start a new renew timer.

[aaronbee]

Another advantage here is that it provides a place for us to add a metric to track the number of live renewals. It can be done in a follow on change, but we will want to add a metric that increments when this function starts and decrements when it exits. That way we can track if we are leaking scanners.

[tanay-vakharia]

Thanks, this definitely cleans up the code and makes it easier to add metrics in the future. I've implemented this, feel free to take a look!

[timoha]

Just my 2c:
What's preventing scanning go routine to keep scanning and storing in a buffer end purposefully renewing the lease? I think the lease should only be renewed purposefully if you are actively scanning data.

Also, please make sure to not have this behavior by default as it might be unexpected for some users, as now their "periods of starvation" in client layer will cause leaked snanners within regionservers.

[aaronbee]

Just my 2c: What's preventing scanning go routine to keep scanning and storing in a buffer end purposefully renewing the lease? I think the lease should only be renewed purposefully if you are actively scanning data.

Also, please make sure to not have this behavior by default as it might be unexpected for some users, as now their "periods of starvation" in client layer will cause leaked snanners within regionservers.

Hey Andrey!
Yeah, I agree this shouldn't be enabled by default inside gohbase.
We have some needs for keeping a long lived scanner running. Buffering extra data is not great because of the extra memory use.

[timoha]

Hey Aaron!
Got it, it makes sense as an option. Just another alternative I'd suggest in those cases is just increasing lease timeout (server or client config). I would expect that eventually you would have to timeout anyway with your scan