lua-resty-etcd icon indicating copy to clipboard operation
lua-resty-etcd copied to clipboard

Move require_serializer into v3

Open u5surf opened this issue 2 years ago • 4 comments

  • Fix #126
  • cause to conform to v3.md

Signed-off-by: u5surf [email protected]

u5surf avatar Oct 11 '22 21:10 u5surf

hi @u5surf , can you show how to get the error like #126

tzssangglass avatar Oct 12 '22 02:10 tzssangglass

@tzssangglass I consider that #126's user might use require("resty.etcd.v3") .new instead of require("resty.etcd").new like this.

    location /t {
        content_by_lua_block {
            local cjson = require("cjson.safe")
            local etcd, err = require("resty.etcd.v3").new({
                timeout = 5,
                http_host = "http://127.0.0.1:2379",
                ttl = -1,
                api_prefix = "/v3",
                serializer = "raw"
            })
            check_res(etcd, err)
...

In this case, opt.serializer = "raw" | "json" then it had errored. Thus I added the test case to use require("resty.etcd.v3") .new. So that both of require("XXX").new become to be able to select the serializer type by string which follows the docs, It should be better to select the type of serializer in lib/resty/etcd/v3.lua.

u5surf avatar Oct 12 '22 03:10 u5surf

So that both of require("XXX").new become to be able to select the serializer type by string which follows the docs, It should be better to select the type of serializer in lib/resty/etcd/v3.lua.

I've thought about it for a while and I don't think this is a good change.

Reason:

  1. the require_serializer function in etcd.lua not in v3.lua, this is because the lib originally supported etcd's v2 protocol, so require_serializer was shared by v2.lua and v2.lua. Although this lib is now dropping support for etcd's v2, perhaps etcd's v4 may be available in the future.
  2. require("resty.etcd.v3").new. is the wrong usage and does not need to be compatible.

tzssangglass avatar Oct 12 '22 07:10 tzssangglass

@tzssangglass OK, I understand what you said.

@findmark Which the etcd is referred on your configuration require("resty.etcd") or require("resty.etcd.v3") ? We could not see which it referres on your comments. https://github.com/api7/lua-resty-etcd/issues/126#issuecomment-805406629 if there is require("resty.etcd"), it might have another bug what we could find yet. Else, if there is require("resty.etcd.v3"), your configure seems to be incorrect.

u5surf avatar Oct 12 '22 08:10 u5surf