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

一些加强易用性的修改 #571

Closed
wants to merge 11 commits into from
Closed

Conversation

hqwrong
Copy link
Contributor

@hqwrong hqwrong commented Feb 8, 2017

以下所有修改都是兼容的:

  1. snaxd增加一个查询当前服务名字的方法

  2. snaxd interface 支持定制loader(增加这个功能是因为我需要在加载snax的时候就把服务所在目录加到loader路径里,而不是load完后再加。)

  3. 为cslave增加watch方法,这个在正常关闭服务器的时候有用.

  4. snax hotfix 递归的patch_func.

比如考虑如下场景:

local lobster

local function bar()
-- ...
    local _ = lobster -- reference lobster here
-- .....
end

function response.foo()
  -- ...
    bar()
    -- ...
end

这里我要热更新bar函数,需要让bar函数join老的lobster,之前那样是办不到的。

@hqwrong hqwrong changed the title Formal 一些加强易用性的修改 Feb 8, 2017
@cloudwu
Copy link
Owner

cloudwu commented Feb 8, 2017

其它没有意见,但是 "snaxd增加一个查询当前服务名字的方法" 这个特性我认为实现的味道不好,是硬加了一个判断,且是在 dispatcher 这个根分发函数中。

我建议把查询名字放到 system 子类下,和 init 这些同级。修改 snax.interface 的实现,为每个服务默认加上返回名字的接口(同时可以允许用户自己定义这个接口覆盖)。ps. 可能需要调整 snaxd 里面 SERVICE_NAME = snax_name 到 require 之前。

@hqwrong
Copy link
Contributor Author

hqwrong commented Feb 9, 2017

按你的意思改好了,增加了一个snax.name()函数,不过实现稍微有点不一样。

因为调用这个函数的场景是拿handle查询name,再snax.bind为一个snax对象,即调用的时候不能通过snax obj 来获取name方法的method id, 为此我新建了snax/sysfunc.lua 的文件。

@@ -70,6 +65,9 @@ skynet.start(function()
return profile_table
end)
init = true
elseif command == "name" then
local func = method[4] or function () return SERVICE_NAME end
skynet.ret(skynet.pack(func()))
Copy link
Owner

Choose a reason for hiding this comment

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

这里直接写 skynet.ret(skynet.pack(SERVICE_NAME)) 就够了啊,不需要生成一个匿名函数再调用一次。

lualib/snax.lua Outdated
@@ -1,5 +1,6 @@
local skynet = require "skynet"
local snax_interface = require "snax.interface"
local sysfunc = require"snax.sysfunc"
Copy link
Owner

Choose a reason for hiding this comment

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

从节省开销的角度,这里 sysfunc 只被 snax.name 一个方法调用,应该移到 snax.name 里面去 require 。

@cloudwu
Copy link
Owner

cloudwu commented Feb 9, 2017

我明白了,你要的是从一个 handle 不提供 type 的情况下 bind 成一个对象。并不需要查询 name 。所以我建议不要提供 snax.name 方法。直接在 snax.bind 里加这个特性,当 type 为 nil 的时候,向对方查询。

不过,我有一个疑问:如果你不知道对象的 type ,怎么知道该调用什么方法呢?目前并没有提供 enum 所有方法的 api 。

@hqwrong
Copy link
Contributor Author

hqwrong commented Feb 9, 2017

我只是调用它的系统方法。

具体应用场景是,我为debug_console增加了一个hotfix命令,接受服务地址和hotfix文件路径。我希望这个命令自己去查询它的name.

@cloudwu
Copy link
Owner

cloudwu commented Feb 9, 2017

目前的 sysfunc 的改法是不对的。因为 system 里的字符串和 id 的对应关系是由每个类型自己生成的,并不保证所有服务的对象,相同的 system 方法对应的 id 都是一致的。比如在 snax.hotfix 和 snax.kill 里,就是按类型去取 id 的。

你提到一个独立位置会造成冲突。

我觉得你这个需求更简单的方法是给 launcher.lua 加一个方法,可以获取 handle 对应的启动字符串更好。也就是增加和 function command.LIST() 类似的方法,去取一个特定 handle 的字符串,而不是全部返回。

另外,cslave 的修改是多余的。https://github.com/cloudwu/skynet/wiki/Cluster 见 harbor.link 方法。

@cloudwu
Copy link
Owner

cloudwu commented Feb 9, 2017

如果暂时不给 launcher.lua 加指令的话,其实现在你也可以在 debug console 里调用 LIST 取得所有的,然后挑选出对应的服务启动串的,这样就完全不用修改。

btw. 我个人觉得 snax 的封装有点厚重,可以看一下 https://github.com/cloudwu/skynet_sample 这里写的一个跟简单的封装。

@hqwrong
Copy link
Contributor Author

hqwrong commented Feb 9, 2017

咦 按照这个写法, system func的id 不应该固定是1-4吗?

	local system = { "init", "exit", "hotfix", "profile"}

	do
		for k, v in ipairs(system) do
			system[v] = k
			func[k] = { k , "system", v }
		end
end

@cloudwu
Copy link
Owner

cloudwu commented Feb 9, 2017 via email

@hqwrong
Copy link
Contributor Author

hqwrong commented Feb 9, 2017

因为launcher只记录了同一节点内的服务,如果是要跨节点查询的话,就比较麻烦了。

@cloudwu
Copy link
Owner

cloudwu commented Feb 9, 2017 via email

@cloudwu
Copy link
Owner

cloudwu commented Feb 9, 2017 via email

@hqwrong
Copy link
Contributor Author

hqwrong commented Feb 10, 2017

好的,我把cslave和snax.name的修改去掉了,重新发起了一个pr

#573

@hqwrong hqwrong closed this Feb 10, 2017
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.

2 participants