Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v7: breaking changes you would like to see #1025

Closed
vmihailenco opened this issue May 16, 2019 · 20 comments
Closed

v7: breaking changes you would like to see #1025

vmihailenco opened this issue May 16, 2019 · 20 comments
Labels

Comments

@vmihailenco
Copy link
Collaborator

I am planning v7 release which should support context.Context and which will be incompatible with v6 in one or another way. So if you have in mind some other incompatible changes that make sense - you are welcome to add them here.

One such change I've just made is changing ZAdd from ZAdd(key string, members ...Z) *IntCmd to ZAdd(key string, members ...*Z) *IntCmd. It saves 1 allocations and is detected by compiler - so why not.

@FlamingTree
Copy link
Contributor

will pool related operation(net.Dial etc.) support context.Context?so we can record related time consumed.

@vmihailenco
Copy link
Collaborator Author

@FlamingTree do you mean passing context to Dialer? Yes, it is possible - see https://github.com/go-redis/redis/pull/1046/files

@pierrre
Copy link

pierrre commented Jun 7, 2019

Proper support for Go modules.
Currently my go.mod uses the +incompatible suffix.
If you want to keep the backward compatibility with older versions of Go (without modules support), you may want to move all the code to a /v7 sub-directory (I don't know if it's a good practice, but it was suggested in the official doc).

@vmihailenco
Copy link
Collaborator Author

@pierrre My main concern is that so far I don't have a single go-mod compatible import in the project I am working on (which has ~50 imports). They are either v0.9999 or incompatible.

@pierrre
Copy link

pierrre commented Jun 8, 2019

@vmihailenco in project that is using Go modules, you can import dependencies that don't do anything special to support Go modules.

I'm currently migrating all the projects at my company:

  • if a dependency is supporting Go modules, then it's fine
  • if a dependency is following semver for version v1 or v0, there is nothing special to do
  • if a dependency is following semver for version v2+ (such as go-redis), then I need to use the +incompatible suffix
  • if a dependency only has a "master" branch, then I'm managing it with v0.0.0 + commit ID

The only issue with go-redis, it that the "update all dependencies to the latest version" command keeps reverting go-redis to v6.15.2 instead of 6.15.3, and I don't understand why...

@vmihailenco
Copy link
Collaborator Author

Try GOPROXY=direct go get github.com/go-redis/redis

@pierrre
Copy link

pierrre commented Jun 8, 2019

GOPROXY=direct go get github.com/go-redis/redis@v6.15.3+incompatible
Result in go.mod: github.com/go-redis/redis v6.15.3+incompatible

go list -m -u all
Result: github.com/go-redis/redis v6.15.3+incompatible [v6.15.2+incompatible] (It wants to downgrade ??????)

GOPROXY=direct go get github.com/go-redis/redis
Result in go.mod: github.com/go-redis/redis v6.15.2+incompatible

@FlamingTree
Copy link
Contributor

@FlamingTree do you mean passing context to Dialer? Yes, it is possible - see https://github.com/go-redis/redis/pull/1046/files

I mean it's better to have hook point on specific operation, like getConn from pool, make new connection, etc. Just like stdlib's net/http/httptrace.

@zikaeroh
Copy link
Contributor

zikaeroh commented Jun 17, 2019

For what it's worth, Go at tip (so what will be 1.13) doesn't have the downgrade issue, probably due to golang/go#30634 and a few other module fixes. My gotip list -m -u all:

github.com/go-redis/redis v6.15.2+incompatible [v6.15.3+incompatible]

EDIT: This might actually be a bug in and of itself; I don't think module-aware projects should have +incompatible versions.

@vmihailenco
Copy link
Collaborator Author

I mean it's better to have hook point on specific operation, like getConn from pool, make new connection, etc. Just like stdlib's net/http/httptrace.

I'd like to support something like this at some point, but there is nothing backwards incompatible so this change can wait until later.

@nanasi880
Copy link

nanasi880 commented Jun 25, 2019

I want you to call internal.Logf with context.
Because, I injected custom logger to context.
Logger is mutable by HTTP request metadata. Not a static. ( e.g. logger have HTTP request resource path, logger adding log to parent log by resource path )

@vmihailenco
Copy link
Collaborator Author

Because, I injected custom logger to context.

@nanasi880 do you have an example of relatively popular lib that is doing this? I wonder if there are any existing conventions here.

@nanasi880
Copy link

nanasi880 commented Jun 27, 2019

@vmihailenco
thanks for reply.

I wonder if there are any existing conventions here.

I think so modern library is better to propagate context.
Library do not have to use context.
But, if library logic is long running or create multiple goroutine, It is better to handling ctx.Done()

do you have an example of relatively popular lib that is doing this?

I known only a little.

It is appengine/log package
https://github.com/golang/appengine/blob/master/log/api.go#L18

this package is required appengine's context.
appengine log package usage.

import (
    "context"
    "google.golang.org/appengine"
    "google.golang.org/appengine/log"
)

func httpHandler(w http.ResponseWriter, r *http.Request) {
    var ctx context.Context = appengine.NewContext(r) // create context. returned context is capture HTTP request.
    log.Debugf(ctx, "hi") // output to stackdriver : "hi"

    // invalid call
    // this will panic
    // first parameter is must be appengine's context, and context must have valid HTTP request.
    log.Debugf(context.Background(), "Oops")
}

appengine's log is flush to stackdriver.
stackdriver will grouping log by HTTP resource path and trace id.
for that, logging library need metadata from context.

If, internal.Logf call with context. I can logging which HTTP request is the problem.

like this

func httpHandler(w http.ResponseWriter, r *http.Request) {
    ctx := appengine.NewContext(r)

    // if problem happen, internal.Logf can call with ctx
    err := redisClient.Set(ctx, "my-key", 100, -1).Err()
}

@dcormier
Copy link
Contributor

I'd like to see #616 done, somehow.

@Me1onRind
Copy link

Because, I injected custom logger to context.

@nanasi880 do you have an example of relatively popular lib that is doing this? I wonder if there are any existing conventions here.

https://github.com/jinzhu/gorm support logger hook. Provide a non-authoritative idea, My company's php redis lib only logging command and key , time consuming, and error.

@catkins
Copy link

catkins commented Nov 25, 2019

Is v7 likely to come out in the next few months? starting a new project and considering whether to start out on the beta versions or to stick with stable for now?

@catkins
Copy link

catkins commented Feb 12, 2020

👋 Hi @vmihailenco same question as above – I notice that v7 is still in beta, and I was updating some shared libraries we use, and was weighing up with whether to hop on the v7 train or stick to v6.

@vmihailenco
Copy link
Collaborator Author

Just tagged v7.0.0 - enjoy :)

@catkins
Copy link

catkins commented Feb 12, 2020

Perfect timing! Thanks @vmihailenco 👏

Copy link

This issue is marked stale. It will be closed in 30 days if it is not updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants