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

fix v1 handler: use create command when test against prevexistence #247

Merged
merged 4 commits into from
Oct 18, 2013

Conversation

xiang90
Copy link
Contributor

@xiang90 xiang90 commented Oct 17, 2013

No description provided.

@xiang90
Copy link
Contributor Author

xiang90 commented Oct 17, 2013

@benbjohnson This should fix the single node test.

@philips
Copy link
Contributor

philips commented Oct 17, 2013

@xiangli-cmu Travis seems upset still

--- FAIL: TestRemoveNode (1.00 seconds)
panic: runtime error: index out of range [recovered]
    panic: runtime error: index out of range
goroutine 10 [running]:
testing.func·004()
    /home/travis/.gvm/gos/go1.1/src/pkg/testing/testing.go:348 +0xcd
_/home/travis/gopath/src/github.com/coreos/etcd/tests/functional.TestRemoveNode(0xc2000cc5a0)
    /home/travis/gopath/src/github.com/coreos/etcd/tests/functional/remove_node_test.go:34 +0xc24
testing.tRunner(0xc2000cc5a0, 0x891d98)
    /home/travis/.gvm/gos/go1.1/src/pkg/testing/testing.go:353 +0x8a
created by testing.RunTests
    /home/travis/.gvm/gos/go1.1/src/pkg/testing/testing.go:433 +0x86b
goroutine 1 [chan receive]:
testing.RunTests(0x74fdd8, 0x891d20, 0xa, 0xa, 0x7fff0f263b00, ...)
    /home/travis/.gvm/gos/go1.1/src/pkg/testing/testing.go:434 +0x88e
testing.Main(0x74fdd8, 0x891d20, 0xa, 0xa, 0x896da0, ...)
    /home/travis/.gvm/gos/go1.1/src/pkg/testing/testing.go:365 +0x8a
main.main()
    _/home/travis/gopath/src/github.com/coreos/etcd/tests/functional/_test/_testmain.go:61 +0x9a
goroutine 2 [syscall]:
exit status 2
FAIL    _/home/travis/gopath/src/github.com/coreos/etcd/tests/functional    1.023s
The command "./test.sh" exited with 1.
Done. Your build exited with 1.

@xiang90
Copy link
Contributor Author

xiang90 commented Oct 17, 2013

@philips That tests call exec. And seem like exec failed due to cannot find the binary.

@benbjohnson
Copy link
Contributor

@xiangli-cmu The code is not compiling the binary. I'm getting this error when running build:

src/github.com/coreos/etcd/server/v1/set_key_handler.go:4: imported and not used: "fmt"

The build.sh script doesn't fail when a command fails. I'll add a set -e on the script in a separate pull request to fix that. Another option is to move to Makefiles.

http://stackoverflow.com/questions/2870992/automatic-exit-from-bash-shell-script-on-error/2871029#2871029

@benbjohnson
Copy link
Contributor

@xiangli-cmu Added the -e flag in #248.

if response.PrevValue == "" {
response.NewKey = true
}
}

if response.Action == CompareAndSwap || response.Action == Create {
response.Action = "testAndSet"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiangli-cmu Is this set to testAndSet for backwards compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
Create is test against prevValue="" in v1

@benbjohnson
Copy link
Contributor

@xiangli-cmu Logically, it seems like there should just be a SetCommand and that CAS (by value and by existence) are simply properties of SetCommand. I think we're going to get a lot of duplicate code between the different commands down the road as we add features. That's a whole different discussion though.

Outside of the design question of SetCommand, the code in this PR looks good to me.

@xiang90
Copy link
Contributor Author

xiang90 commented Oct 17, 2013

@benbjohnson We do not have many operation inside the store. Command is just wrapper. But we can even cleanup the wrappers in the future.

xiang90 added a commit that referenced this pull request Oct 18, 2013
fix v1 handler: use create command when test against prevexistence
@xiang90 xiang90 merged commit 2eff77e into etcd-io:0.2 Oct 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants