Closed jacinpoz closed 7 years ago
Modifying existing lua instances is tricky because lua is not thread-safe. Also, InstanceQueue does not contain lua instances that are actively being used in query aggregations. I think this logic would be unstable when considering all use-cases.
If the user understands these risks, then cache clearing/unloading could be called directly by the user when appropriate. Here are slightly modified versions of clearPackage() or clearPackages():
public static final void clearPackage(String packageName) {
// This method assumes Lua instances are not currently being used.
// Active lua instances will not have their packages unloaded.
Prototype prototype = Packages.remove(packageName);
if (prototype != null) {
for (final LuaInstance luaInstance : InstanceQueue){
luaInstance.unloadPackage(packageName);
}
}
}
public static final void clearPackages() {
// This method assumes Lua instances are not currently being used.
// Active lua instances will not be cleared.
InstanceQueue.clear();
Packages.clear();
}
If that makes sense, I can include this code in the next client release.
I agree with you and I was fully aware of the fact that this solution would not be thread-safe, and that InstanceQueue could be missing some of the instances when I implemented it.
But even if the logic is unstable in this sense, the current implementation just does not handle removing/registering or registering/registering the same lua package in a correct fashion, it does not work as expected. If the package has new functions, they won't be accessible after the new registration and there is no way to fix this with the current API.
I think it makes sense and it is better to let the user decide when it is safe to call clearPackage() or clearPackages() and make sure the caveats of the current implementation are known in the documentation and/or Javadocs.
Thanks for your consideration for the next release 👍
Ok. These methods will be added in the next release.
Java client 3.3.2 has been released:
I have several integration tests in which I register a lua file, let's call it "mylua", but sometimes I need to re-register it with extra functions in different tests (as it is generated dynamically), or simply remove it.
However, LuaCache contains pre-cached
Prototype
(s) andLuaInstance
(s) for performance reasons I presume, but whenremoveUdf
is called inAerospikeClient
or a newregister
operation happens, the actual references inside all theLuaInstance
(s) are still pointing to the previously compiled "mylua" and therefore trying to call new functions from the new "mylua" file will fail with "function not found".I have seen a
LuaCache.clearPackages()
method inLuaCache
, and I thought would fix my problem. But even though we clear thePrototype
(s) map, the previously createdLuaInstance
(s) are still cached, and every time aQueryAggregateExecutor
callslua.loadPackage(statement)
it will not reload the new file as this code snippet checks if it was already loaded or not and the response is always true:if (loadedTable.get(statement.getPackageName()).toboolean()) { return; }
Therefore I extended the classes AerospikeClient.java, LuaCache.java and LuaInstance.java, so the
removeUdf
method clears the package fromLuaCache
and also sets the load value to false for each package in eachLuaInstance
object. I also extended clearPackages to clear theLuaInstance
queue as well, as any instance in the queue will be "corrupted" and still referencing old non-existent lua files.It definitely helps testing, and I believe this is also the way
removeUdf
method should really work.