storage: preserve original error on function fail #629
Conversation
| instance_name = 'named_replica' | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
Nit: the commit msg is a little bit misleading, I didn't manage to understand the problem from it:
For older Tarantool versions vshard uses func.call to execute functions.
Nah, we use custom local_call function for older versions, which uses func.call for functions defined in the box.schema.func and net.box.self.call for everything else
So, at the end only box.schema.func are affected by this problem?
There was a problem hiding this comment.
This sentence is still incorrect:
For older Tarantool versions vshard uses func.call to execute functions registered in schema.
Let's properly write it. I'm asking you to do that, since any user or developer in the future may read this commit msg, let's not mislead them
kamenkremen
force-pushed
the
gh-614
branch
4 times, most recently
from
4ecada4 to
949d8a9
Compare
For older Tarantool versions vshard uses func.call to execute functions registered in schema. If such a function raises an error during an uncommitted transaction, 'Transaction is active...' error is going to be raised, which will mask the original error. This patch fixes that bug by checking the function call result. If the function returns an error and a transaction is still active, vshard now explicitly rolls back the transaction. Additionally, this patch fixes inconsistent return types. Previously, functions registered in the schema returned raw cdata (such as tuples), while other functions returned data converted via msgpack. This patch ensures consistency by performing a msgpack conversion for registered functions as well. Fixes tarantool#614 Fixes tarantool#630 NO_DOC=bugfix
Serpentian
left a comment
Serpentian
left a comment
There was a problem hiding this comment.
Almost there! Well done
| end | ||
| end | ||
|
|
||
| local function handle_results(status, ...) |
There was a problem hiding this comment.
Nit: let's add a comment, that this is copy-pasta from net_box.lua, so that we don't forget that in the future
| local function handle_results(status, ...) | ||
| if not status then | ||
| box.rollback() | ||
| return status, ... |
There was a problem hiding this comment.
Why not box.error.new(box.error.PROC_LUA, (...))? This leads to inconsistencies:
The diff for example:
diff --git a/example/storage.lua b/example/storage.lua
index 05e5ef3..9cea354 100755
--- a/example/storage.lua
+++ b/example/storage.lua
@@ -123,5 +123,6 @@ function raise_luajit_error()
end
function raise_client_error()
- box.error(box.error.UNKNOWN)
+ -- box.error(box.error.UNKNOWN)
+ box.error('CustomConnectionError', 'cannot connect to the given port')
end
On tarantool 3:
unix/:./data/router_1.control> vshard.router.callro(1, 'raise_client_error')
---
- null
- trace: [{'file': '[C]', 'line': 4294967295}]
code: 32
base_type: ClientError
type: ClientError
details: cannot connect to the given port
name: PROC_LUA
message: cannot connect to the given port
...
On tarantool 2:
unix/:./data/router_1.control> vshard.router.callro(1, 'raise_client_error')
---
- null
- code: 0
base_type: CustomError
type: CustomConnectionError
custom_type: CustomConnectionError
message: cannot connect to the given port
trace: [{'file': './storage_2_a.lua', 'line': 127}]
...
| ilt.assert_equals(result1, result2, {100, 200, 'tuple'}) | ||
| box.schema.func.drop('test_return_tuple1') | ||
| rawset(_G, 'test_return_tuple1', nil) | ||
| rawset(_G, 'test_return_tuple2', nil) |
There was a problem hiding this comment.
Nit: you can drop the variables with _G.test_return_tuple1 = nil, but we cannot set them like that, due to luacheck.
| instance_name = 'named_replica' | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
This sentence is still incorrect:
For older Tarantool versions vshard uses func.call to execute functions registered in schema.
Let's properly write it. I'm asking you to do that, since any user or developer in the future may read this commit msg, let's not mislead them
|
|
||
| test_group.test_local_call_return_type_consistency = function(g) | ||
| g.replica_1_a:exec(function() | ||
| rawset(_G, 'test_return_tuple1', function() |
There was a problem hiding this comment.
And why don't we wanna use templates here? Two identical functions look strange
2 participants
For older Tarantool versions vshard uses func.call to execute functions.
If such a function raises an error during an uncommitted transaction, 'Transaction is active...' error is going to be raised, which will mask the original error.
This patch fixes that bug by checking the function call result. If the function returns an error and a transaction is still active, vshard now explicitly rolls back the transaction.
Fixes #614
Fixes #630
NO_DOC=bugfix