-
Notifications
You must be signed in to change notification settings - Fork 35
storage, router: allow calling info when disabled #622
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
storage, router: allow calling info when disabled #622
Conversation
Serpentian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations on your first PR! Good work, I'm impressed, you've managed to come this far without help. We have to work a little bit more on the patch, check out my comments below
731ff3f to
080db8f
Compare
Serpentian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very, very good! The patch is already very clean, let's polish it a little bit more
080db8f to
393db21
Compare
393db21 to
989cc85
Compare
Serpentian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! This was fast, good work
Gerold103
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch 💪! I particularly like the way you managed to split it into commits, quite easy to read.
SetupHardware: Tarantool: 2 shards, 2 replicas in one shard, 2 routers, 4 load generators, which're Mean by 20 iterations: The perf test is from the https://github.com/tarantool/vshard-ee/pull/51. Before patchset: 172084.8500 (100%) Nah, Vlad is right, perf degradate a lot, 3% is way too much. Let's go another way here, instead of making router use explicit args, let's make storage use varargs: The patchdiff --git a/vshard/storage/init.lua b/vshard/storage/init.lua
index 9764225..9b8ecd1 100644
--- a/vshard/storage/init.lua
+++ b/vshard/storage/init.lua
@@ -4020,15 +4020,15 @@ end
-- Arguments are listed explicitly instead of '...' because the latter does not
-- jit.
--
-local function storage_api_call_safe(func, arg1, arg2, arg3, arg4)
- return func(arg1, arg2, arg3, arg4)
+local function storage_api_call_safe(func, ...)
+ return func(...)
end
--
-- Unsafe proxy is loaded with protections. But it is used rarely and only in
-- the beginning of instance's lifetime.
--
-local function storage_api_call_unsafe(func, arg1, arg2, arg3, arg4)
+local function storage_api_call_unsafe(func, ...)
-- box.info is quite expensive. Avoid calling it again when the instance
-- is finally loaded.
if not M.is_loaded then
@@ -4054,12 +4054,12 @@ local function storage_api_call_unsafe(func, arg1, arg2, arg3, arg4)
return error(lerror.vshard(lerror.code.STORAGE_IS_DISABLED, msg))
end
M.api_call_cache = storage_api_call_safe
- return func(arg1, arg2, arg3, arg4)
+ return func(...)
end
local function storage_make_api(func)
- return function(arg1, arg2, arg3, arg4)
- return M.api_call_cache(func, arg1, arg2, arg3, arg4)
+ return function(...)
+ return M.api_call_cache(func, ...)
end
endAfter the patch above in the above mentioned setup: 171938.0000 (99.9%). Pretty much the same, but this is because we test it in the case of router overload (storages are not at 100%). Let's check it in another setup: SetupHardware: same. Tarantool: same. 2 shards, 2 replicas in one shard, 9 routers, Before the patch above: 204278.4000 (100%) That's good, let's go this way instead, the router's and storages will be consistent, |
989cc85 to
d373701
Compare
557a835 to
883cada
Compare
Serpentian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the patches! Clean work! Just one small nit, sorry, didn't notice it on prev iteration of review.
17dd39c to
197c026
Compare
|
CI failed due to flaky tests in router_2_2_test. The fix is submitted in PR #625 |
Currently, internal storage API functions use explicit arguments. This differs from the router API implementation, where varargs are used. This patch replaces explicit arguments with varargs in the storage_api_call_safe, storage_api_call_unsafe and storage_make_api functions. Motivation: - Consistency between router and storage internal APIs - Improved performance: using varargs is more efficient than passing explicit arguments Needed for tarantool#565 NO_DOC=refactoring NO_TEST=refactoring
A storage may be marked as disabled, which previously made it difficult to obtain status information from the component. This patch allows calling vshard.storage.info even when the component is disabled. The function now also returns a new boolean field is_enabled indicating whether the component is currently enabled. Closes tarantool#565 @TarantoolBot document Title: vshard: storage.info behavior change vshard.storage.info() This function can now be called even when the storage is disabled. It now also returns a new boolean field is_enabled indicating whether the component is currently enabled.
A router may be marked as disabled, which previously made it difficult to obtain status information from the component. This patch allows calling vshard.router.info even when the component is disabled. The function now also returns a new boolean field is_enabled indicating whether the component is currently enabled. Follow-up tarantool#565 @TarantoolBot document Title: vshard: router.info behavior change vshard.router.info() This function can now be called even when the router is disabled. It now also returns a new boolean field is_enabled indicating whether the component is currently enabled.
197c026 to
4bda356
Compare
A storage or router may be marked as disabled, which previously made it difficult to obtain status information from these components.
This patch allows calling
vshard.storage.infoandvshard.router.infoeven when the component is disabled. Both functions now also return a new boolean fieldis_enabledindicating whether the component is currently enabled.Closes #565
@TarantoolBot document
Title: vshard: info functions behavior change
vshard.storage.info()
vshard.router.info()
These functions can now be called even when the storage or router is disabled.
Both functions now also return a new boolean field
is_enabledindicating whether the component is currently enabled.