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

Move require_serializer into v3 #176

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

u5surf
Copy link
Contributor

@u5surf u5surf commented Oct 11, 2022

Signed-off-by: u5surf [email protected]

lib/resty/etcd/v3.lua Outdated Show resolved Hide resolved
@tzssangglass
Copy link
Contributor

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

@u5surf
Copy link
Contributor Author

u5surf commented Oct 12, 2022

@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.

@tzssangglass
Copy link
Contributor

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 tzssangglass self-requested a review October 12, 2022 07:10
@u5surf
Copy link
Contributor Author

u5surf commented Oct 12, 2022

@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.
#126 (comment)
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

something wrong with decode json kv.value
2 participants