Add support for transactions #6

Pryz · 2017-05-22T02:00:15Z

No description provided.

achille-roussel · 2017-05-22T02:08:32Z

client.go

 return ParseArgs(c.Query(ctx, cmd, args...), nil)
 }

+func (c *Client) MultiExec(ctx context.Context, cmds ...Command) error {


We need documentation for this method.

achille-roussel · 2017-05-22T02:09:00Z

client.go

 }

+r, err := c.Do(&Request{
+Addr: addr,


This can be rewritten

Cms: []Command{{ ... }}

(no need to repeat Command{)

achille-roussel · 2017-05-22T02:09:27Z

client.go

+
+return r.Args
+}
+


Same here, we need documentation for this method.

achille-roussel · 2017-05-22T02:10:06Z

conn_test.go

-tx1 := makeConnTx(ctx, &Request{Cmd: "SET", Args: List("redis-go.test.conn", "0123456789")})
-tx2 := makeConnTx(ctx, &Request{Cmd: "GET", Args: List("redis-go.test.conn")})
+req1 := &Request{
+Cmds: []Command{Command{Cmd: "SET", Args: List("redis-go.test.conn", "0123456789")}},


Here we can get rid of the Command as well.

achille-roussel · 2017-05-22T02:10:09Z

conn_test.go

+tx1 := makeConnTx(ctx, req1)
+
+req2 := &Request{
+Cmds: []Command{Command{Cmd: "GET", Args: List("redis-go.test.conn")}},


Here we can get rid of the Command as well.

achille-roussel · 2017-05-22T02:10:55Z

request.go

-// For server request, Args is never nil, even if there are no values in
-// the argument list.
-Args Args
+// Cmds is the list of transaction's commands.


Cmds is the list of transaction's commands. => Cmds is the list of commands that are part of the transaction.

achille-roussel · 2017-05-22T02:11:29Z

request.go

-return ParseArgs(req.Args, dsts...)
+for _, cmd := range req.Cmds {
+err := ParseArgs(cmd.Args, dsts...)
+if err != nil {


Let's inline the error check here if err := ParseArgs(cmd.Args, dsts...); err != nil {

achille-roussel · 2017-05-22T02:11:58Z

server.go

-Addr: conn.RemoteAddr().String(),
-Args: args,
-Context: ctx,
+cmd = Command{


How about doing cmd := Command{ here and get rid of the variable declaration above?

achille-roussel · 2017-05-22T02:12:53Z

server.go

 }

+cmds := []Command{}
+cmds = append(cmds, cmd)


Here we could shorten the code by rewriting it as cmds := []Command{cmd}.

achille-roussel · 2017-05-22T02:14:29Z

server_test.go

+defer cancel()
+
+err := cli.MultiExec(ctx,
+Command{Cmd: "MULTI"},


So how about making MULTI and EXEC automatic?

achille-roussel · 2017-05-25T01:20:12Z

client.go

+if len(addr) == 0 {
+addr = "localhost:6379"
+}
+


You can optimize the memory allocations here by rewriting the code as

multi := make([]Command, 0, len(cmds)+2) multi = append(multi, Command{Cmd: "MULTI"}) multi = append(multi, cmds...) multi = append(multi, Command{Cmd: "EXEC"})

achille-roussel · 2017-05-25T01:20:40Z

client.go

+addr := c.Addr
+if len(addr) == 0 {
+addr = "localhost:6379"
+}


Let's check that the list of command doesn't contain any MULTI, EXEC or DISCARD.

What should we do in that case ? drop the query or remove duplicate keys ?

I think it's fine to drop the query and return an error, it's a misuse of the package.

achille-roussel · 2017-05-25T01:21:32Z

request.go

-// For server request, Args is never nil, even if there are no values in
-// the argument list.
-Args Args
+// Cmds is a lir of commands that are part of the transaction.


achille-roussel · 2017-05-25T01:22:21Z

server.go


 func (s *Server) serveRequest(res *responseWriter, req *Request) (err error) {
-args := req.Args
+cmds := req.Cmds


It seems like this intermediary variable isn't useful, we could just write for _, cmd := range req.Cmds {

achille-roussel · 2017-05-25T01:23:49Z

server.go

-args.Close()
-}
+default:
+err = s.serveRedis(res, req)


This is weird isn't it? It's gonna call serveRedis multiple times for the same request if there are many arguments.

Pryz · 2017-07-10T02:39:11Z

So this is mostly working except when we parse the response.

Sending :

MULTI GET foo GET bob EXEC

Will return

+OK +QUEUED +QUEUED *2 $3 bar $5 alice

But we only have +OK in the Args. I'll check how we are dealing with that in the resp parser I guess.

achille-roussel · 2017-07-10T21:42:56Z

conn_test.go

+}
+}
+
+func isInArray(list []string, val string) bool {


This function doesn't seem used.

achille-roussel · 2017-07-10T21:43:24Z

response.go

-if err := enc.Encode(val); err != nil {
-return err
+for _, arg := range res.Args {
+if arg != nil {


Can this ever happen? It seems like it's a bad idea to allow nil arguments.

achille-roussel · 2017-07-10T21:43:45Z

response.go

 return enc.Close()
 }

+func (res *Response) CloseAllArgs() error {


How about we just call this Close?

yeah I was just lazy of writing a for loop multiple times :)

I think the method is fine, I was just arguing on the method name.

My bad :) Can changed that for sure !

Pryz · 2017-07-10T22:05:40Z

args.go

 err := args.dec.Err()

-if args.done != nil {
+if args.done != nil && err != nil {


@achille-roussel not sure this one is correct

Nope, we have to notify the done channel even if there were no errors, otherwise each connection will only be able to handle one request, then will timeout.

achille-roussel · 2017-07-14T06:07:12Z

Closing in favor of #26 Thanks for your hard work on that one man!

Pryz requested a review from achille-roussel May 22, 2017 02:00

achille-roussel reviewed May 22, 2017

View reviewed changes

Pryz force-pushed the support-transactions branch 3 times, most recently from c644eb2 to 336e701 Compare May 24, 2017 21:38

achille-roussel reviewed May 25, 2017

View reviewed changes

Pryz force-pushed the support-transactions branch 2 times, most recently from 31e503a to 1eb106a Compare July 10, 2017 02:34

achille-roussel reviewed Jul 10, 2017

View reviewed changes

Pryz force-pushed the support-transactions branch 2 times, most recently from dce0e01 to 0915048 Compare July 10, 2017 22:04

Pryz commented Jul 10, 2017

View reviewed changes

Pryz force-pushed the support-transactions branch 5 times, most recently from 4264009 to 47bff1f Compare July 10, 2017 23:31

Support transactions

b7df68a

Pryz force-pushed the support-transactions branch from 47bff1f to b7df68a Compare July 11, 2017 01:38

Fix conn test

8881cdb

Pryz force-pushed the support-transactions branch from 073d590 to d3248b5 Compare July 11, 2017 03:05

fix typo

f6649ec

Pryz force-pushed the support-transactions branch from d3248b5 to 9afcdf4 Compare July 11, 2017 07:24

clean up

1176824

Pryz force-pushed the support-transactions branch from 9afcdf4 to 1176824 Compare July 11, 2017 07:26

achille-roussel mentioned this pull request Jul 14, 2017

add support for transactions #26

Merged

achille-roussel closed this Jul 14, 2017

achille-roussel deleted the support-transactions branch July 14, 2017 06:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add support for transactions #6

Add support for transactions #6

Uh oh!

Pryz commented May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 22, 2017

achille-roussel May 25, 2017

achille-roussel May 25, 2017

Pryz Jul 11, 2017

achille-roussel Jul 11, 2017

achille-roussel May 25, 2017

achille-roussel May 25, 2017

achille-roussel May 25, 2017

Pryz commented Jul 10, 2017

achille-roussel Jul 10, 2017

achille-roussel Jul 10, 2017

achille-roussel Jul 10, 2017

Pryz Jul 10, 2017

achille-roussel Jul 10, 2017

Pryz Jul 10, 2017

Pryz Jul 10, 2017

achille-roussel Jul 10, 2017

achille-roussel commented Jul 14, 2017

Labels

3 participants

Uh oh!

Add support for transactions #6

Add support for transactions #6

Uh oh!

Conversation

Pryz commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pryz commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achille-roussel commented Jul 14, 2017

Labels

3 participants