Update code to fix almost all pre-existing lint errors

[zendern]

What this PR does:
Cleanups all but one of the lint errors that were found when the hash was in place.

Last outstanding issue is as follows

pkg/querier/frontend/worker.go:60:10: SA1019: naming.Watcher is deprecated: please use package resolver.  (staticcheck)
	watcher naming.Watcher

Not 100% clear yet on path forward on this. From what i can tell even the underlying library is still using naming.Watcher. Wondering if for now we put a //nolint:staticcheck and open an issue to fix just that issue.

Which issue(s) this PR fixes:
Fixes #1912

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[zendern]

@jtlisi let me know if you have feeling or thoughts on how to handle the last outstanding lint issue. Tagged you since you were the original issue reporter.

[jtlisi]

LGTM, just a minor nit. Also for that final linting error, feel free to add //nolint:staticcheck and open an issue.

Thanks for taking this on

[zendern]

Opened #2015 to deal with that last outstanding lint issue. All checks are now passing. @jtlisi should be ready for you to take a look at again.

[bboreham]

Great work! I made a lot of comments; I think the FramedSnappy one is a real issue.

[bboreham]

I question why Store() has no error return.

[jtlisi]

FWIW from pkg/chunk/cache/cache.go

// Cache byte arrays by key.
//
// NB we intentionally do not return errors in this interface - caching is best
// effort by definition.  We found that when these methods did return errors,
// the caller would just log them - so its easier for implementation to do that.
// Whats more, we found partially successful Fetchs were often treated as failed
// when they returned an error.
type Cache interface {
	Store(ctx context.Context, key []string, buf [][]byte)
	Fetch(ctx context.Context, keys []string) (found []string, bufs [][]byte, missing []string)
	Stop() error
}

[zendern]

@bboreham just double checking are we good with this?? as per the comment above it was very intentional.

[pracucci]

Good job @zendern. Looking at the change set, and the possible issues spotted, it's easy to understand the benefits of the linter. I've also left some comments.

[pracucci]

When used in conjunction with NewBackoffRetry() (see grpcclient.go), the backoff expects a ResourceExhausted error code.

@bboreham what do you think returning that GRPC error here?

[bboreham]

That sounds like a bug.
"Error: I'm not going to wait because the context will expire before the wait finishes"
"Never mind, try again!"

[zendern]

So @bboreham / @pracucci if i am understanding correctly what you are saying is that making this change is the wrong thing to do. Basically since the wait would not return a ResourceExhausted that would cause the BackoffRetry, if its being used, to now exit and no longer retry like we expect it to.

So if that is correct the right course of action is to just go back to what we had in originally here
#2008 (comment)
and ignore the error or should the BackoffRetry code check for another error code along with ResourceExhausted.

[pracucci]

I would not rollback but fix it. We may do:
return status.New(codes.ResourceExhausted, err.Error())

But I would be glad to also see a unit test on it.

[zendern]

I'll look at adding a unit test for this and getting that change in place.

[pracucci]

Ok thanks! This looks like the very last thing left to me.

[zendern]

@pracucci So i wrote up this unit test last night.....which might not be enough honestly. It's fine that it can validate that its a status with the expected result but an integration test would probably drive home the fact that it's necessary if you configure it with the backoffRetry. I just ran out of time.....I'll see if i can get to it over the weekend.

import (
	"context"
	"testing"

	"github.com/cortexproject/cortex/pkg/util/grpcclient"
	assert2 "github.com/stretchr/testify/assert"
	"google.golang.org/grpc"
	"google.golang.org/grpc/codes"
	"google.golang.org/grpc/status"
)

func TestRateLimiterFailureResultsInResourceExhaustedError(t *testing.T) {
	config := grpcclient.Config{
		RateLimitBurst: 0,
		RateLimit: 0,
	}
	conn := grpc.ClientConn{}
	invoker := func(currentCtx context.Context, currentMethod string, currentReq, currentRepl interface{}, currentConn *grpc.ClientConn, currentOpts ...grpc.CallOption) error {
		return nil
	}

	limiter := grpcclient.NewRateLimiter(&config)
	err := limiter(context.Background(), "methodName", "", "expectedReply", &conn, invoker)
	
	if se, ok := err.(interface {
		GRPCStatus() *status.Status
	}); ok {
		assert2.Equal(t, se.GRPCStatus().Code(), codes.ResourceExhausted)
		assert2.Equal(t, se.GRPCStatus().Message(), "rate: Wait(n=1) exceeds limiter's burst 0")
	} else{
		assert2.Fail(t, "Could not convert error into expected Status type")
	}
}

[zendern]

@bboreham @pracucci so looking into golangci and errcheck i dont believe I am able to add the excludes on (hash.Hash).Write as of now. golangci has forked errcheck and is using their own version.

Using errcheck directly the above exclude works b/c they have built in support for embedded interfaces. Whereas the golangci fork of errcheck is out of date and does not have that code.

https://github.com/golangci/errcheck/tree/master/internal/errcheck

https://github.com/kisielk/errcheck/tree/master/internal/errcheck

I'm working on a PR to try to bring the golangci-lint version up to the same as errcheck master but that will take a little time. So how would you like to proceed??

[pracucci]

so looking into golangci and errcheck i dont believe I am able to add the excludes on (hash.Hash).Write as of now.
So how would you like to proceed??

Personally I'm fine even without an override if it's not easy to do. All in all, it affect 3 places and we can always re-iterate on it once supported.

[jtlisi]

Since the scope is limited, I'm fine without the override as well

[pracucci]

Is there any specific reason why this function was introduced? It looks a bit superfluous to me, given we can directly call require.NoError(t, fixture.Teardown()) instead of TeardownFixture().

[zendern]

It was done in this commit. 8bf4365 It was just something i noticed we were doing 2 places so felt like a natural thing since the fixture was already in the testutils.go class. I could go either way....ill leave that up to you but that was the reasoning.

[pracucci]

I would not rollback but fix it. We may do:
return status.New(codes.ResourceExhausted, err.Error())

But I would be glad to also see a unit test on it.

[pracucci]

@zendern Could you attempt another rebase and address the open comment, please? I'm hearing your pain continuously rebasing this: I'm going to stress other maintainers to review this as well, so that we can get this merged.

[bboreham]

Excellent work, thanks!
Let's merge it quick before anyone else blinks.

[pracucci]

I agree, awesome work! Let's merge this! 🎉