search

couchbase / sync_gateway

Manages access and synchronization between Couchbase Lite and Couchbase Server
https://www.couchbase.com/products/sync-gateway
Other
448 stars 138 forks source link

Refactor SG repo to remove need to modify GOPATH + use go tools directly #1572

Closed ajres closed 8 years ago

ajres commented 8 years ago

Golang 1.5 introduced experimental support for including packages stored in a ./vendor folder for a project.

In Golang 1.6 this will be enabled by default, and in Golang 1.7 it will no longer be possible to disable it via environment variables.

Support for this feature is slowly being added to GO IDE's and IDE plugins.

We should refactor the SG repo to support this new feature and simplify using SG code with IDE's and standard tools. For example SG currently highjacks the GOPATH variable (see build.sh).

These are the areas that need to be considered for migration:

ajres commented 8 years ago

Golang 1.6 has an approximate release date of Feb 2016.

I suggest we make Go 1.6 a dependency of this refactor based on these considerations:

  1. Vendor folder support is enabled by default, there is no need to define the ''GO15VENDOREXPERIMENT' environment variable.
  2. No new language features are introduced , so existing plugins and IDE's should not be impacted.
  3. Transparent support for HTTP/2 is introduced, which should improve sync performance.
tleyden commented 8 years ago

Here's a project that uses vendoring: https://github.com/keybase/client/blob/f1da498b176bcfe5793eedb6893f6f2c5890ba76/go/notes/vendor.md

tleyden commented 8 years ago

Good article: https://blog.gopheracademy.com/advent-2015/vendor-folder/

tleyden commented 8 years ago

Some vendoring tools

Tool Notes
gvt
govendor
ajres commented 8 years ago

We probably want to use a third party tool to maintain a persistent description of our project dependencies so that developers can easily populate the vendor folder after checkout.

Some tools:

  1. govendor
  2. Glide
  3. More...
ajres commented 8 years ago

One feature to consider for package management tools is 'import path rewrites '.

This should allow us to reference the upstream dependency project repos, but actually pull from the couchbasedeps forks.

ajres commented 8 years ago

So far only direct third party dependencies have been forked to couchbasedeps. I think sub-dependencies may also need to be forked.

Not sure how the pkg manager tools will handle nested dependencies.

ajres commented 8 years ago

Looks like GOPATH must still be set to include the project root folder, can we drop the GOPATH highjacking in build.sh?

tleyden commented 8 years ago

I think sub-dependencies may also need to be forked.

Yeah, this turns into a bit of a rabbit hole. Say we import foo, which in turn imports bar.

With a tool like gvt (and possibly others), at least with their default mode of operation there seems to be no need to fork 3rd party repos -- it grabs a complete copy of their source and puts it into the vendor folder. (ditto for transitive dependencies)

tleyden commented 8 years ago

can we drop the GOPATH highjacking in build.sh?

Yeah I think that should be a hard requirement

tleyden commented 8 years ago

One feature to consider for package management tools is 'import path rewrites '.

Hoping we can avoid that

tleyden commented 8 years ago

Looks like travis-ci/worker uses gvt without checking in vendored source. Not necessarily suggesting we go that route, just making a note of it.

tleyden commented 8 years ago

Added wiki page: https://github.com/couchbase/sync_gateway/wiki/Sync-Gateway-1.3-Vendoring-Options

tleyden commented 8 years ago

In the issue_1572_restructure_repo branch I've made the following changes as an experiment:

Building the code via go build and bringing up sync gateway via go run is working, but if I try to run the tests I'm getting a few panics:

https://gist.githubusercontent.com/tleyden/68552282fe9a3504a2cf/raw/c14a6026ee56eb3dd6e19aa50fcc2fa549cc04a5/gistfile1.txt (search for "panic")

The first one might be an incompatibility with the latest version of Otto (and in fact, that was the library that caused us to do the submodule vendoring in the first place)

But this one is puzzling (and looks familiar):

--- FAIL: TestUserViewQuery (0.18s)
panic: interface conversion: interface is []string, not []interface {} [recovered]
    panic: interface conversion: interface is []string, not []interface {}

goroutine 6877 [running]:
testing.tRunner.func1(0xc822d64360)
    /usr/local/go/src/testing/testing.go:450 +0x171

I guess we must have submodules on the master branch that are pointing to commits that aren't the latest master branch commits for dependent libraries like sg-bucket or walrus.

tleyden commented 8 years ago

I guess we must have submodules on the master branch that are pointing to commits that aren't the latest master branch commits for dependent libraries like sg-bucket or walrus.

Or maybe I have things on my $GOPATH that are old and out of date.

tleyden commented 8 years ago

I think I found the culprit, when I pinned otto to commit f9e07770bd9b5142de517b05a85ffbb42d1895da, both of those errors disappeared.

I'm now getting a new failure:

https://gist.github.com/tleyden/4c855b46b83f672ec42c#file-gistfile1-txt-L5471

2016-02-10T02:05:54.019Z HTTP:  #1135: POST http://localhost/db/_online  (ADMIN)
2016-02-10T02:05:54.021Z HTTP:  #1136: GET http://localhost/db/  (ADMIN)

    admin_api_test.go:772
    assert.True(t, body["state"].(string) == "Online")
--- FAIL: TestDBOnlineConcurrent (0.03s)
    assert.go:31:
2016/02/10 02:05:54 Taking DB offline
2016-02-10T02:05:54.022Z Opening db /db as bucket "sync_gateway_test_17", pool "default", server <walrus:>
2016-02-10T02:05:54.022Z Opening Walrus database sync_gateway_test_17 on <walrus:>
tleyden commented 8 years ago

I'm now getting a new failure: FAIL: TestDBOnlineConcurrent

Nevermind, that's only happening under docker. When running directly on OSX I'm not seeing the issue. (test.sh -race -cpu 4 also passes)

tleyden commented 8 years ago

I'm trying use gopkg.in to point to the older otto commit corresponding to a v1.0 tag I added on our fork, but getting stuck on this error:

$ go build
# github.com/couchbaselabs/sync_gateway/channels
channels/sync_runner.go:114: cannot use func literal (type func("gopkg.in/couchbaselabs/otto.v1".FunctionCall) "gopkg.in/couchbaselabs/otto.v1".Value) as type sgbucket.NativeFunction in argument to runner.JSRunner.DefineNativeFunction
tleyden commented 8 years ago

Found the issue -- sg-bucket is also importing otto, and they were importing different ottos ..

tleyden commented 8 years ago

@snej / @jchris -- do you guys have any recollection of what the nature of the incompatibility of Sync Gateway with newer versions of Otto? If it was a bug in Otto .. was there ever a bug filed against Otto? Asking because it would be nice to able to point to the latest master commit of Otto, but I'm currently getting test failures when trying to do that.

zgramana commented 8 years ago

@tleyden that looks like a reflection bug. Either Otto or SG might be trying to get/set a field it thinks is a struct that was changed to a string.

snej commented 8 years ago

To be honest, I have no recollection of there being bugs/incompatibilities in Otto. I think I just didn't update Otto anymore after a while, because the version we had worked.

tleyden commented 8 years ago

Not sure if it's the same issue, but I did find https://github.com/robertkrimen/otto/issues/28 (filed by @snej), related to https://github.com/couchbase/sync_gateway/issues/89

tleyden commented 8 years ago

I noticed the following panic when running the functional tests via py.test -s -k “test_users_channels"

panic: runtime error: slice bounds out of range

goroutine 42 [running]:
github.com/couchbase/gomemcached.(*MCRequest).Receive(0xc820423280, 0x7fb2c5088550, 0xc820024e08, 0xc820437d40, 0x18, 0x18, 0x0, 0x0, 0x0)
    /workspace/src/github.com/couchbase/gomemcached/mc_req.go:181 +0xbcc
github.com/couchbase/gomemcached/client.(*Client).runFeed(0xc820429ad0, 0xc82043b140, 0xc820437d20)
    /workspace/src/github.com/couchbase/gomemcached/client/tap_feed.go:270 +0x149
created by github.com/couchbase/gomemcached/client.(*Client).StartTapFeed
    /workspace/src/github.com/couchbase/gomemcached/client/tap_feed.go:251 +0x428

Note: there were also errors when running the same functional tests against the master branch of Sync Gateway, however I didn't notice any panics (which doesn't mean there wasn't any). Having trouble interpreting the functional test output from that run.

tleyden commented 8 years ago

Might be related to the fact that the couchbaselabs/sync_gateway is using this gomemcached commit:

commit c4dcd702418948a2263c111970919035e6a4fd75
Author: Eben Haber <eben@couchbase.com>
Date:   Tue Feb 9 12:31:26 2016 -0800

versus couchbase/sync_gateway using a much older commit:

commit 43416aaf844678e55421b5f6451cca50fec02993
Author: manik <manik.taneja@gmail.com>
Date:   Tue Oct 20 00:08:14 2015 +0530

For go-couchbase, it's a similar situation. couchbaselabs/sync_gateway is using 

commit 3ea76d43d1f33fa657c8092b89b77accdc8305b6
Author: sitaramv <sitaram.vemulapalli@couchbase.com>
Date:   Wed Feb 10 11:07:54 2016 -0800

vs couchbase/sync_gateway using:

commit ac7dc184fbbd3b89205ba9152ad59ae2996d36c9
Author: manik <manik.taneja@gmail.com>
Date:   Mon Oct 19 21:24:08 2015 +0530
adamcfraser commented 8 years ago

It does look like https://github.com/couchbase/gomemcached/commit/f4caed221d0550525f11a843e22855b6582ef116 introduced the code that's panicking (and is after the commit sync_gateway was using). It looks suspiciously like a change that was made to support DCP, that isn't backwards-compatible for TAP.

tleyden commented 8 years ago

@adamcfraser I'm going to look into the repo based build manifest which would allow building of the couchbaselabs/sync_gateway proof-of-concept repo with dependency pinning to the same dependency versions that couchbase/sync_gateway is locked down to

tleyden commented 8 years ago

I built couchbaselabs/sync_gateway with this manifest, which has the gomemcached and go-couchbase repos pinned to the same commits that couchbase/sync_gateway is using, and built the code using these commands:

Get repo tool

$ curl https://storage.googleapis.com/git-repo-downloads/repo > /usr/bin/repo
$ chmod +x /usr/bin/repo

Init repo

$ repo init -u "https://github.com/tleyden/sync_gateway_manifest.git" -m default.xml

Repo sync

$ repo sync

Build

$ export GOPATH=/root/godeps
$ cd src/github.com/couchbaselabs/sync_gateway
$ go install

And kicked off a py.test -s -k "test_users_channels" functional test.

tleyden commented 8 years ago

Here are the fuctional test results when building via repo tool with pinned dependencies:

https://gist.github.com/tleyden/7ea395c8891f1c7781ce

I didn't notice any panics (which doesn't mean that no panics happened). It looks like the overall numbers weren't as good as the previous test from the sync gateway master branch.

sync gateway master branch

========== 4 failed, 4 passed, 88 deselected, 1 pytest-warnings, 4 error in 2134.98 seconds ==========

built via repo w/ pinned dependencies

===================================================== 5 failed, 3 passed, 88 deselected, 1 pytest-warnings, 5 error in 2171.51 seconds =====================================================

Given all the "connection timeout" issues, not sure if it's a valid regression or just sporadic behavior. @sethrosetter any help in interpreting the output would be appreciated!

tleyden commented 8 years ago

I didn't notice any panics (which doesn't mean that no panics happened)

I just verified that there were no panics in that last functional test run:

[root@33d065f7b4c0 tmp]# zipgrep panic /tmp/functional_tests-test_users_channels.py-test_multiple_users_multiple_channels\[DI-1\]-2016-02-12-16-32-28-sglogs.zip
[root@33d065f7b4c0 tmp]# zipgrep panic /tmp/functional_tests-test_users_channels.py-test_multiple_users_multiple_channels\[CC-2\]-2016-02-12-16-34-22-sglogs.zip
[root@33d065f7b4c0 tmp]# zipgrep panic /tmp/functional_tests-test_users_channels.py-test_muliple_users_single_channel\[CC-2\]-2016-02-12-16-39-46-sglogs.zip
[root@33d065f7b4c0 tmp]# zipgrep panic /tmp/functional_tests-test_users_channels.py-test_single_user_single_channel\[DI-1\]-2016-02-12-16-55-11-sglogs.zip
[root@33d065f7b4c0 tmp]# zipgrep panic /tmp/functional_tests-test_users_channels.py-test_single_user_single_channel\[CC-2\]-2016-02-12-17-05-46-sglogs.zip
tleyden commented 8 years ago

I verified that go get -u -v -t github.com/couchbase/sync_gateway/... works on a clean system:

https://gist.github.com/tleyden/3e4bd822f1ad76777543