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(clustering/rpc): support jsonrpc notification #13948

Merged
merged 44 commits into from
Dec 18, 2024

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Nov 29, 2024

Summary

KAG-5893

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix #[issue number]

@github-actions github-actions bot added core/clustering cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Nov 29, 2024
@chronolaw chronolaw changed the title feat(clusering/rpc): support jsonrpc notify feat(clustering/rpc): support jsonrpc notify Nov 29, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 29, 2024
@chronolaw chronolaw force-pushed the feat/support_jsonrpc_notify branch from 33fe9ec to e77cafc Compare November 29, 2024 13:15
@chronolaw chronolaw changed the title feat(clustering/rpc): support jsonrpc notify feat(clustering/rpc): support jsonrpc notification Nov 30, 2024
@chronolaw chronolaw force-pushed the feat/support_jsonrpc_notify branch from 509a30d to 2bfecec Compare December 3, 2024 12:29
@chronolaw chronolaw marked this pull request as ready for review December 3, 2024 22:46
@@ -151,7 +162,7 @@ function _M:start()
ngx_log(ngx_DEBUG, "[rpc] got RPC call: ", payload.method, " (id: ", payload.id, ")")

local dispatch_cb = self.manager.callbacks.callbacks[payload.method]
if not dispatch_cb then
if not dispatch_cb and payload.id then
Copy link
Contributor

@chobits chobits Dec 5, 2024

Choose a reason for hiding this comment

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

From the Json-RPC 2.0 spec, I think the reply MUST contain id field. So could we check whether there is a payload.id right after the line local payload = decompress_payload(data)

Copy link
Contributor

Choose a reason for hiding this comment

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

like

local payload = decompress_payload(data)

-- check id firstly
if not payload.id then
   return error_log("not found id")
end

if type(payload.id) ~= number then
   return error_log("invalid id: not a number")
end

-- From now on, we assure that payload.id exists and it is a number
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the examles in json rpc spec:

--> {"jsonrpc": "2.0", "method": "update", "params": [1,2,3,4,5]}
--> {"jsonrpc": "2.0", "method": "foobar"}


return nil, fut.error.message
end


function _M:notify(node_id, method, ...)
Copy link
Contributor

@chobits chobits Dec 5, 2024

Choose a reason for hiding this comment

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

diff with _M:call(node_id, method, ...), I think we could combine them into one

@@ -1,4 +1,4 @@
-function _M:notify(node_id, method, ...)
+function _M:call(node_id, method, ...)
   local cap = utils.parse_method_name(method)

   local res, err = self:_find_node_and_check_capability(node_id, cap)
@@ -30,8 +30,25 @@ function _M:notify(node_id, method, ...)
   assert(res == "concentrator")

   -- try concentrator
-  local fut = future.new(node_id, self.concentrator, method, params, true)
+  local fut = future.new(node_id, self.concentrator, method, params)
   assert(fut:start())

-  return true
+  local ok, err = fut:wait(5)
+
+  if err then
+    ngx_log(ngx_DEBUG, "[rpc] ", method, " failed, err: ", err)
+
+    return nil, err
+  end
+
+  if ok then
+    ngx_log(ngx_DEBUG, "[rpc] ", method, " succeeded")
+
+    return fut.result
+  end
+
+  ngx_log(ngx_DEBUG, "[rpc] ", method, " failed, err: ", fut.error.message)
+
+  return nil, fut.error.message
 end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored.

@chronolaw
Copy link
Contributor Author

We should add test cases for it later.

@chronolaw chronolaw force-pushed the feat/support_jsonrpc_notify branch from dda12a1 to 3d6f84f Compare December 16, 2024 09:54
@chronolaw chronolaw force-pushed the feat/support_jsonrpc_notify branch from cd08089 to 047fcf9 Compare December 17, 2024 08:17
@ADD-SP ADD-SP merged commit be7e356 into master Dec 18, 2024
26 checks passed
@ADD-SP ADD-SP deleted the feat/support_jsonrpc_notify branch December 18, 2024 06:14
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

@chronolaw chronolaw restored the feat/support_jsonrpc_notify branch January 10, 2025 08:38
@chronolaw chronolaw deleted the feat/support_jsonrpc_notify branch January 10, 2025 08:39
ADD-SP pushed a commit that referenced this pull request Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/clustering size/L skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants