fix(native): Hoist plugin loading and de-inline factory map for dynamic connector support#156
Open
20001020ycx wants to merge 1 commit into
Open
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d8e4993 to
f7d3730
Compare
62db030 to
abf1515
Compare
abf1515 to
8e60013
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
registerDynamicFunctions()beforeregisterVeloxConnectors()inPrestoServer.cppRegistration.hintoRegistration.cppMotivation and Context
This PR enables dynamic connector plugins — connectors loaded at runtime via
plugin-dirusingdlopen()— as described in RFC-0019: Connector Plugins. Two changes are needed to make this work correctly.Both changes revolve around a singleton factory map defined in
Registration.cpp:static std::unordered_map<std::string, std::shared_ptr<ConnectorFactory>> factories;This map lives inside
detail::connectorFactories()and maps connector names (e.g."hive","tpch") to theirConnectorFactoryinstances. BothregisterConnectorFactory(name, factory)andgetConnectorFactory(name)read and write this map.Why
registerDynamicFunctions()must be hoisted beforeregisterVeloxConnectors()registerVeloxConnectors()iterates over every*.propertiesfile in thecatalog/directory. For each one, it readsconnector.nameand callsgetConnectorFactory(connectorName)to look up the factory in the singleton map, then callsfactory->newConnector(...)to instantiate the connector.registerDynamicFunctions()loads plugin.sofiles from theplugin/directory viadlopen(). Each plugin'sregisterExtensions()entry point callsregisterConnectorFactory(...)to insert its factory into the same singleton map.Before this PR,
registerVeloxConnectors()ran beforeregisterDynamicFunctions(). Since plugins had not been loaded yet, the factory map had no entry for dynamically registered connector names. WhenregisterVeloxConnectors()processed a catalog .properties file referencing a dynamic connector,getConnectorFactory()threw because the corresponding factory had not been registered yet.So the sequence must be:
registerDynamicFunctions()— loads plugin.sofiles, plugins callregisterConnectorFactory()to insert into the mapregisterVeloxConnectors()— readscatalog/*.properties, callsgetConnectorFactory()to look up entries in the mapThe original code had no problem with built-in connectors (Hive, TPC-H, etc.) because their factories are registered statically at compile time.
Why registration functions must be de-inlined
Before this PR, the factory map was a static local variable inside the inline function connectorFactories() in Registration.h. Any connector shared library that #includes this header gets its own copy of the static variable, meaning both presto_server and the plugin library hold separate factory maps (Note, shared library references the header file in the presto-native-execution for compilation, the implementation are resolved at run time):
This breaks the singleton property of the factory map: the plugin's
registerConnectorFactory()inserts into its own map (0xBBBB), but the server'sgetConnectorFactory()reads from its own map (0xAAAA) — and finds nothing.De-inlining connectorFactories() ensures the static local variable exists in exactly one place (Registration.cpp, linked into presto_server), allowing dynamically registered connector known to presto_server successfully.
Test Plan
presto_serverwith the changesRelease Notes