vespa icon indicating copy to clipboard operation
vespa copied to clipboard

contributing vespaclient in go by <Get.It>

Open renevall opened this issue 2 years ago • 8 comments

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

Vespa Go Client Proposal

For the past 8-12 months we have been building a Go Client for Vespa that we planned to share/donate/open source to Vespa and its community. We believe this is only a fraction of the work to be done, but it has served us well in production to date and we believe it is something we can build on top of. We are proud to say that despite any potential shortcoming, anyone could use it in the current state to work with vespa's /api/v1/document api.

The current implementation was inspired by olivere/elastic library, where a key element of the API is method chaining to describe a request. This does conflict with the regular way go does error checking and bubbling technique, but in exchange, the API to our eyes seemed straightforward to follow. The errors are still properly exposed and can be handled by the end-user.

The most basic use-case for search would look like:

var opts []vespa.ClientOptionFunc
connStr := "http://localhost:8080"
opts = append(opts,
  vespa.SetURL(connStr),
)

client, err := vespa.NewClient(ctx, opts...)
if err != nil {
  panic(err)
}

res, err := vespa.NewSearchService(client).
  Scheme("docs").
  Query("Trucking Class B").
  Where("userQuery() AND status contains \"active\" AND geoLocation(location, 33.64, -84.33, \"25mi\") AND hidden = false").
  Timeout("5s").
  Ranking(vespa.Ranking{Profile: "bm25"}).
  Do(ctx)

The current implementation supports the following:

  • Create
  • Read
  • Update
  • Partial Update
  • Delete
  • Search
  • Batch Create, Update.
  • HTTP2 support through official http2 implementation

One of the known shortcomings is that the Batch process sends one request per document, essentially making it a fake batch operation. What it does provide instead is an abstraction to a batch operation, where each request launches its own routine and they are synchronized again with the use of a wait-group. Good enough for anyone to use and get up to seen 3000qps for feed operations.

As stated, we don't believe this implementation is perfect by any means, but it has done the job for us for a while. We hope this becomes either something to build on or inspiration for the official library in the future.

Thanks all,

Get.it Team

renevall avatar Oct 13 '21 03:10 renevall

@mpolden can you review this?

On module structure, my thought here was to move the current client/go to client/go/cli and then put all of this in client/go/api, as this is "a Go API to Vespa". Open to other suggestions if you have any.

bratseth avatar Oct 13 '21 09:10 bratseth

I haven't looked at in this in detail yet, but I think the following would be a good structure:

  1. Move all Go code to a top-level go directory as client is already used for something else.
  2. Move CLI code to go/internal - marking it as private API, as that code is not intended for direct use by anything else. https://golang.org/doc/go1.4#internalpackages
  3. Put this code in go/vespa as api is not a good package name. See https://go.dev/blog/package-names.
  4. Maybe later: Change CLI to use this API where possible.

I can take care of 1 and 2.

mpolden avatar Oct 13 '21 11:10 mpolden

1.: Separating by language instead of by functional module at the top level seems a step in the wrong direction to me. We have lots of modules with multiple languages in them already - why do something special here? (Could be a sufficiently good reason exists and I just don't see it.)

2: "internal" doesn't seem like a good name for the CLI? Why not call it cli? What happens if we get other internal stuff? Or did you mean go/internal/cli?

  1. Aren't you mixing go package names and vespa module names here? Presumably this has multiple go packages in it, but that whole thing still needs to be called something (I think) in Vespa, and "vespa" seems strange to me. If nobody likes API we could do "goapi", "goclient", or ...?

bratseth avatar Oct 13 '21 11:10 bratseth

  1. A single workspace simplifies building and inter-dependencies. If we ever introduce Go code that does not fit under client we have to introduce multiple Go modules / workspaces.

I don't see a point in multiple modules when everything is in a single repository. go get github.com/vespa-engine/vespa/<something> clones the repo regardless. When pulling dependencies into your Go code, only the code you actually reference end up in the final binary so it's not an issue from that perspective.

  1. Yes, we can have whatever structure we want under internal. The point is that private APIs are under that path.

  2. Aren't you mixing go package names and vespa module names here?

How are they different? Maven modules are closer to Go packages when consuming them in an external project. Remember that the final component of the import path becomes the name the user consumes. Consider

import github.com/vespa-engine/vespa/client/go/api

func main() {
  client := api.NewClient(...) // Without reading the import statement, what on earth is this?
  client.Query(api.NewQuery(...))
  // or client := goapi.NewClient()
  // or client := goclient.New()
}

vs.

import github.com/vespa-engine/vespa/client/go/vespa

func main() {
  client := vespa.NewClient(...)
  client.Query(vespa.NewQuery(...))
}

The implementation of vespa.NewClient may of course use code from a package named client and NewQuery from one named yql etc.

This also allows reusing private APIs between sub-packages, e.g. yql and client may both want to use code from internal/{pkg1,pkg2,pkg3}, which aren't public.

mpolden avatar Oct 13 '21 13:10 mpolden

On second thought, keeping everything under client/go should be fine for now. I do think the package name for external use should be vespa though.

mpolden avatar Oct 13 '21 13:10 mpolden

I do think the package name for external use should be vespa though.

Agreed, and ensuring that is more important than our internal structure.

By "vespa modules" (as opposed to go packages) I just meant the directory names: Below client/go we'll have two different things (the cli and the api) that will both have a set of go packages and will both produce a binary.

If we have this structure, then both those things (being directories) will need to have names and those names will not be visible in the go code, which just sees the go packages at the level below.

bratseth avatar Oct 13 '21 13:10 bratseth

@renevall It appears you have added the source files in both client/go/api/ and vespaclient-go/. Can you remove the latter? The files are mostly the same, except a few differences in the examples so you have to decide which ones to keep:

$ diff -u <(git show --stat HEAD|grep client/go/api | awk '{print $1}' | xargs sha256sum | sed 's|client/go/api/||' | sort -k 2) <(git show --stat HEAD|grep vespaclient-go | awk '{print $1}' | xargs sha256sum | sed 's|vespaclient-go/||' | sort -k 2)
--- /dev/fd/11	2021-10-18 13:30:55.118235109 +0200
+++ /dev/fd/12	2021-10-18 13:30:55.118942330 +0200
@@ -1,6 +1,5 @@
 1f9b498b3f856c5cf52c8a1545c9d163f6f606f85e16ded20bba81ee17aafbc3  .gitignore
 d04a59d254535a92861c1b460d2ce1f8bee177242015a3d6e4f0296f6924998d  .golangci.yml
-dda3eccbd816756661680d43492af14f5a968701e69c849f1f8d81a53b5f7d8a  Makefile
 167ec6a37e94174c99b0696bc747290bb2fa9ee020ad32f4bea048fa825085ab  README.md
 3095bf93abf75d8e2005e1be3b3ac77338e664e28575fdda6b92252cd7588688  Vespa.md
 6f79f50132abf155aac9c3f8c59a256f2294885d2c4892e1fc692bbe949145c3  batch.go
@@ -15,14 +14,14 @@
 1773ea0fd643d9459d547a3bd3285cc626ef85e75bfd45b79239bc828b40983c  create_test.go
 39b9600c1d07b33eb330fbd9e321b6feef382848c5f46c302dbf51d2b861f5d2  delete.go
 b9518a1195812db12fffd5161c3c0ece36238d67ecb54fb12ae58a3fe3d38dec  delete_test.go
-952e98cff2628334961255aa37f76d93de87ee44f1c5f2bd6c412c34159a657e  examples/delete/delete.go
-833d5db7fa6b99e0bf713eac1ba18a8a10fa9162c6166deb557c845961b8ea32  examples/export/export.go
-54b81495a74d668532c082b39a9a70892e53b492a4700be4fe0beb1d2b5b23a0  examples/get/get.go
-0f3911dd2e47463c02b255719c4a05fd11713abd30f072a2eaa9e29921598d11  examples/insert/insert.go
-f6e18e849b74b8adefc19a019a51f91404b2dc340480ff6dc246d4da9aa15e34  examples/mtls/mtls.go
-ee8a80ee43b533f330d784e2c8975d3267fd0e490f23f83ec82d931090a3397f  examples/search/search.go
-63a5d113401a2d766f0fe93b9661b19a3e3cfe1ec92498446ef214971bca074c  examples/update/update.go
-2ee246cb35b7207c194de131db7da44fd572c774f2b57ce35d82218232f403dd  examples/visit/visit.go
+91e7e0109c6a5cb2d61306b80883c3d178182b72238b72502666f1a3fbe0177b  examples/delete/delete.go
+2ea341988725071e22cdd8c24b6720f282bcaaae14695087ffc2b2f71ca26c26  examples/export/export.go
+c0ba3cc063dfba277d52503d604ccff5368cf45e116db7bcf4b64ed2548308c1  examples/get/get.go
+1ebf512a8e068dd501fc24189a8f55cf527307b349cbb5f7e948fbfc11734366  examples/insert/insert.go
+33c5bab0a670dfd54f3fb455b1a3fcc856f09168ce36d0c7f7276ae169c86d51  examples/mtls/mtls.go
+bb0cc7e508761cce681c0852bf873ba8666846baaecdb43d127b60d7df9c34b6  examples/search/search.go
+95f2f021099504328e201a0147485216d4c6715617d2ccd7b8149d595bbfb683  examples/update/update.go
+78bb0b581e0303ffc1f18e472c91e67cf0bc2b3b6b91196548d665d322bb0065  examples/visit/visit.go
 14a2a51d678d361c72536b2d2593092e2a614d40b4190be758300c30aef4a4cf  get.go
 d8a407b89bf035e8a8fa674d7c05b9ad5c4ecbc4658875d1283247bba742564b  get_test.go
 cbcc5727feff1a4311852c9753b9ecda2c4c7c02d898b1217bf0e8ca9f449ac3  go.mod

mpolden avatar Oct 18 '21 11:10 mpolden

thanks @mpolden. I'm not sure how I missed that. I recall amending the commit. Could you check again, I left only client/go/api folder.

renevall avatar Oct 19 '21 01:10 renevall

@bratseth This can probably be closed?

aressem avatar Oct 31 '23 11:10 aressem

@mpolden ?

bratseth avatar Oct 31 '23 11:10 bratseth

Closing for now. Properly supporting a Go API requires a concentrated effort.

mpolden avatar Nov 16 '23 13:11 mpolden