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

server executes only one of two mutations requested #2815

Closed
glycerine opened this issue Dec 9, 2018 · 11 comments
Closed

server executes only one of two mutations requested #2815

glycerine opened this issue Dec 9, 2018 · 11 comments
Assignees
Labels
kind/bug Something is broken.

Comments

@glycerine
Copy link

glycerine commented Dec 9, 2018

  • What version of Dgraph are you using?

The latest as of this writing. In docker dgraph/dgraph:latest

Dgraph version   : v1.0.10
Commit SHA-1     : 8b801bd7
Commit timestamp : 2018-11-05 17:52:33 -0800
Branch           : HEAD
  • What is the hardware spec (RAM, OS)?
    OSX 10.13.6, 16GB ram

  • Steps to reproduce the issue (command/config used to run Dgraph).

image

Given the following two mutations entered into the Mutate tab of Ratel the web gui:

{ set { _:a1 <reg>  "a1 content" . } }
{ set { _:a2 <reg>  "a2 content" . } }

and then Press Run.

  • Expected behavior and actual result:

I expected that both mutations would be run.

However, only the first mutation is run. Any mutations after the first are ignored.

update: reg is defined as

reg: string @index(trigram) @lang .
@MichelDiz
Copy link
Contributor

MichelDiz commented Dec 9, 2018

The syntax is wrong.

{
	set {
		_:a1 < reg > "a1 content" .
		_:a2 < reg > "a2 content" .
	}
}

Ratel does one mutation at a time.

@glycerine
Copy link
Author

glycerine commented Dec 9, 2018

I think this is a bug and should remain open. Ratel should execute all the independent mutations supplied in the text given.

The syntax is correct for two independent transactions.

@glycerine
Copy link
Author

@MichelDiz At the very least this a usability issue. Is certainly surprised me as a first time, new user--and that is a valuable perspective to leverage. Copying transactions from the tour is how I found this. At a minimum, if Ratel is going to ignore some part of the entered text (which is ridiculous), then it should warn the user that part of the user's request is going to be arbitrarily ignored.

@MichelDiz
Copy link
Contributor

Ratel is just a tool. There's no need for dealing with transactions - cuz Ratel commit it immediately. You can't do much with it. If you wanna for some reason do multiple transactions, you must use a client for this. This usage isn't predict for Ratel.

And it does not make much sense within Ratel this feature*. However if you still need this feature, open an issue in Ratel's repository. But notice if there is no duplicate.

@glycerine
Copy link
Author

glycerine commented Dec 9, 2018

Turns out that the problem is on the server rather than with Ratel. Here I reproduce outside of Ratel:

curl -X POST localhost:8080/alter -d '{"drop_all": true}'

{"data":{"code":"Success","message":"Done"}}


curl localhost:8080/alter -XPOST -d 'reg: string @index(trigram) @lang .'

{"data":{"code":"Success","message":"Done"}}

`##` note here that a1 and b2 mutations are both requested:
curl localhost:8080/mutate -XPOST -H 'X-Dgraph-CommitNow: true' \ 
      -d '{ set { _:a1 <reg>  "a1 content" . } } { set { _:b2 <reg>  "b2 content" . } }'

{"data":{"code":"Success","message":"Done","uids":{"a1":"0xf90bb"}},"extensions":{"server_latency":{"parsing_ns":147200,"processing_ns":4289200},"txn":{"start_ts":45732,"commit_ts":45733}}}

curl localhost:8080/query -XPOST -d '{  me2(func: has(reg)) {   uid reg  }}'

`##` note the surprising result here: b2 is lost.  Only a1 has been set.
`##` I consider this a bug. Data loss is always a serious issue.
`##` One could argue that the syntax is wrong, but the server doesn't respond
`##` with a syntax error at all. So even if one downgrades this
`##` bug to being a syntax error, then the server should error out
`##` completely saying 'syntax error', rather than arbitrarily discarding an issued 
`##` command to store data.
{"data":{"me2":[{"uid":"0xf90bb","reg":"a1 content"}]},"extensions":{"server_latency":{"parsing_ns":11200,"processing_ns":2724000,"encoding_ns":796400},"txn":{"start_ts":45734}}}
jaten@Jasons-MacBook-Pro ~ $

@glycerine
Copy link
Author

glycerine commented Dec 9, 2018

Another, distinct example of syntax which results in data loss and no syntax error would be this one. The difference is that I've deleted } { from inside the mutation command:

curl localhost:8080/mutate -XPOST  -H "X-Dgraph-CommitNow: true"  \
   -d $'{ 
               set { _:a1 <reg>  "a1 content" . }  
               set { _:b2 <reg>  "b2 content" . } 
     }'

Here a1 is lost and only b2 is set:

$ curl localhost:8080/query -XPOST -d '{  me2(func: has(reg)) {   uid reg  }}'

{"data":{"me2":[{"uid":"0xf90bd","reg":"b2 content"}]},"extensions":{"server_latency":{"parsing_ns":12600,"processing_ns":2222800,"encoding_ns":764800},"txn"
:{"start_ts":48628}}}

@MichelDiz
Copy link
Contributor

MichelDiz commented Dec 9, 2018

@glycerine this usage ins't predict anywhere in documentation.

This isn't data loss cuz you're using Dgraph in a non-existent way. The "Set" block mutation it is intended to be used only once for each command. If you want two transactions, make two calls.

@srfrog @gitlw what do you guys think? make the same thing as the #2675 ? I don't know if would be good to add a check there or just warn in docs that this usage is wrong.

@glycerine
Copy link
Author

In my humble opinion (and that of the Go authors), "warnings" are bad: we users rarely see them and almost always ignore them. If its going to result in data loss, just error out with a syntax error and force me to find the right syntax. That teaches me what the right syntax is, and I never succeed in doing something that I expect should add data but has actually discarded data.

@glycerine
Copy link
Author

glycerine commented Dec 9, 2018

Scalability wise I really think that multiple independent mutations should be allowed. If I am aggregating multiple mutation requests from different partners, I won't know how to re-combine their requests, because that would require me to parse and re-write their requests. And it would be really slow to have to make a client-server roundtrip per request that I am aggregating. Far better to simply write a simple loop in the server to loop over the submitted text and execute all the transactions found.

I'm happy to add such code in a PR if someone would kindly point me to the source file where the mutation parsing is done. And re-open this bug.

@glycerine glycerine changed the title ratel "run" button only executes the first mutation of the entered mutations server executes only one of two mutations requested Dec 9, 2018
@manishrjain manishrjain reopened this Dec 10, 2018
@manishrjain
Copy link
Contributor

We should indeed return error when there are two set instructions. This is a valid issue. Let's treat it as such.

@dgraph-io dgraph-io deleted a comment from MichelDiz Dec 10, 2018
@manishrjain
Copy link
Contributor

@glycerine if you want to contribute, you can look at gql module. We should return error if the user input does not eof after the block ends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something is broken.
Development

No branches or pull requests

4 participants