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

feat: supports serialization of options #97

Closed
wants to merge 7 commits into from

Conversation

waitingsong
Copy link
Contributor

@waitingsong waitingsong commented Feb 13, 2020

ref: https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#advanced-serialization-for-ipc

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

@waitingsong
Copy link
Contributor Author

@atian25 atian25 requested a review from hyj1991 February 13, 2020 12:22
@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #97 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   98.82%   98.83%   +0.01%     
==========================================
  Files           8        8              
  Lines         511      517       +6     
==========================================
+ Hits          505      511       +6     
  Misses          6        6              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 423726d...1249d64. Read the comment docs.

@waitingsong
Copy link
Contributor Author

waitingsong commented Feb 13, 2020

serialization 在 Node.js < 12.16 时可以传递任何值,不会异常。在 >= 12.16 时非 json advanced 值会抛异常。

另,是否需要在 debug 模式时默认设置为 advanced 值?
或者当 >= 12.16.0 时全部默认 'advanced'?

| https | `Object` | start a https server, note: `key` / `cert` / `ca` should be full path to file |
| require | `Array\|String` | will inject into worker/agent process |
| pidFile | `String` | will save master pid to this file |
| [serialization] | `json|advanced` | default: 'json'. 'advanced' requires Node.js >= 10.16.0 |

Copy link
Member

Choose a reason for hiding this comment

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

这里是 12.16.0 吧?

Copy link
Contributor Author

@waitingsong waitingsong Feb 14, 2020

Choose a reason for hiding this comment

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

另,是否需要在 debug 模式时默认设置为 advanced 值?
或者当 >= 12.16.0 时全部默认 'advanced'?

我看了下下面的相关讨论:nodejs/node#30162 (comment), 看起来 advanced 并不是所有场景下都是最优解,所以这里确实应该 follow 上游的设置,默认为 'json',但是在框架里在适当的版本暴露可配置选项。

我也是如此考虑:随着版本更新性能提升及可靠性得到进一步验证,可能在未来某个版本默认改为 advanced。
所以跟随 node.js 默认设置(json),代码里面不做默认值设置,让有需要的用户自行决定传入参数控制。

README.md Outdated Show resolved Hide resolved
lib/master.js Outdated
@@ -248,6 +250,14 @@ class Master extends EventEmitter {
const debugPort = process.env.EGG_AGENT_DEBUG_PORT || 5800;
if (this.options.isDebug) opt.execArgv = process.execArgv.concat([ `--${semver.gte(process.version, '8.0.0') ? 'inspect' : 'debug'}-port=${debugPort}` ]);

/* istanbul ignore next */
if (semver.gte(process.version, '12.16.0')) {
Copy link
Member

Choose a reason for hiding this comment

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

这里的判断有一个问题,'v13.0.0' 也是 >= 12.16.0,但是这个版本 patch 了此功能么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

对于 12 版本是 >= 12.16.0, 对于 13 版本是 >= 13.2.0。
代码已更新判断条件。

lib/utils/options.js Outdated Show resolved Hide resolved
Copy link
Member

@hyj1991 hyj1991 left a comment

Choose a reason for hiding this comment

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

感谢您关于 v12.16.0 更新的支持更多 IPC 序列/反序列化的配置参数 PR,以上是我的一些对于 PR 代码的修改建议,期望可以一起针对修改/不修改的合理性进行讨论 :)

@hyj1991
Copy link
Member

hyj1991 commented Feb 14, 2020

另,是否需要在 debug 模式时默认设置为 advanced 值?
或者当 >= 12.16.0 时全部默认 'advanced'?

我看了下下面的相关讨论:nodejs/node#30162 (comment), 看起来 advanced 并不是所有场景下都是最优解,所以这里确实应该 follow 上游的设置,默认为 'json',但是在框架里在适当的版本暴露可配置选项。

@atian25
Copy link
Member

atian25 commented Feb 14, 2020

如果这个未来还会变化的话,那目前能否先通过 NODE_OPTIONS ENV 的方式来使用就好了?

@waitingsong
Copy link
Contributor Author

waitingsong commented Feb 14, 2020

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

@atian25
Copy link
Member

atian25 commented Feb 14, 2020

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的,fork 默认是继承 env 的。可以试验下

@waitingsong
Copy link
Contributor Author

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的,fork 默认是继承 env 的。可以试验下

就是说不修改代码,通过设置 ENV 也能起效果。那我关闭这个 PR 了哟?

@atian25
Copy link
Member

atian25 commented Feb 14, 2020

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的,fork 默认是继承 env 的。可以试验下

就是说不修改代码,通过设置 ENV 也能起效果。那我关闭这个 PR 了哟?

我理解是这样的,你实验下看看

@waitingsong
Copy link
Contributor Author

NODE_OPTIONS ENV

这个变量设置是否影响 forkAgentWorker() 对 agent 进程的派生。

影响的,fork 默认是继承 env 的。可以试验下

就是说不修改代码,通过设置 ENV 也能起效果。那我关闭这个 PR 了哟?

我理解是这样的,你实验下看看

那我测试下。

@hyj1991
Copy link
Member

hyj1991 commented Feb 18, 2020

所以这个 PR 还有下文不。。。

@waitingsong
Copy link
Contributor Author

所以这个 PR 还有下文不。。。

么时间测试啊,我先关掉。

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.

3 participants