Plugin Registry #7949

mrambacher · 2021-02-09T23:00:48Z

Added a Plugin class to the ObjectRegistry. Enabled compile-time and program-time addition of plugins to the Registry.

ajkr · 2021-02-10T00:14:02Z

For me to review, could there be a PR at the same scope as PR 7918 (just the mechanism for building-in external code)? I plan to not get involved with plugins beyond that scope.

yhr · 2021-02-11T13:33:12Z

The code generation and plugin->object registry handling looks sane (and would let us drop -whole-archive / -u linking directives which is nice), but do we need versioning checks for builtins? the plugin would be built with the same version of rocksdb, right?

mrambacher · 2021-06-09T20:25:49Z

The code generation and plugin->object registry handling looks sane (and would let us drop -whole-archive / -u linking directives which is nice), but do we need versioning checks for builtins? the plugin would be built with the same version of rocksdb, right?

The "version" check stuff does not necessarily need to be there, until we start using different compilation units (static or shared libraries) to grab the plugin properties. This field is initialized in the struct "for free" and the caller does not need to get/set it.

- Added a "PluginProperties" struct that defines the characteristics of a plugin (version, name, registration). - Added ObjectRegistry::RegisterPlugin. These methods validate a plugin and register it with the ObjectRegistry. - Added builtin plugins to the ObjectRegistry. These are discovered at compile time by looking in the plugin makefile. The builtin plugins are automatically registered with the ObjectRegistry at startup. The builtins are added to the "build_version.cc" when the file is generated. - Added rudimentary support for plugins via cmake.

util/build_version.cc.in

include/rocksdb/version.h

include/rocksdb/utilities/object_registry.h

pdillinger · 2021-11-15T18:22:32Z

include/rocksdb/utilities/object_registry.h

+struct Plugin {
+  // The version of this structure.  If the structure is changed, the version
+  // should be incremented
+  static constexpr int kVersion() { return 1; }


This is unused. What is it doing?

Originally, I had a few versions in this struct:

The RocksDBVersion of the system/tool (hence the earlier changes to RocksDBVersion). RocksDB would pass this value into the Plugin so it could determine that it supported the RocksDB version. If the PluginFunc did not like the RocksDB version, it would return an error

The RocksDB version of the Plugin. This would be filled in by the plugin to say what version it was compiled against. RocksDB would then check that the version was "okay" with it and not add the plugin if it was not

The version of the Plugin struct. This version was to be used by the plugin to state what fields it was filling in and aware of. The the Plugin struct changed, the version of the struct would need to change so that the plugin would know.

pdillinger

This code still seems very haphazard and arbitrary. The tests don't give me many clues about what practical use will look like.

pdillinger · 2021-11-16T17:27:33Z

include/rocksdb/utilities/object_registry.h

@@ -38,6 +39,23 @@ using FactoryFunc =
 // library
 using RegistrarFunc = std::function<int(ObjectLibrary&, const std::string&)>;

+// The signature of the function for loading plugins
+// Plugin contains the struct of properties associated with this Plugin.


This comment still doesn't make sense to me. Is arg a property associated with the Plugin?

pdillinger · 2021-11-16T17:30:33Z

include/rocksdb/utilities/object_registry.h

+// by the Plugin code.
+// On success, this function should return 0.  On failure, this function should
+// return non-zero and set the std::string* to an appropriate error message.
+using PluginFunc = std::function<int(Plugin*, std::string*)>;


And can you tell me what problem is solved by the existence of PluginFunc? You can obviously call RegisterPlugin without it.

The PluginFunc is called to fill in the Plugin* with the appropriate values (at the moment this is only the registration function and its arg).

In the FB-controlled world, this method has one purpose and that is to initialize any plugins defined at compile time during startup. In the rest of the world, I could envision this function being used to register plugins that the user could load dynamically.

include/rocksdb/version.h

pdillinger · 2021-11-29T18:54:07Z

utilities/object_registry.cc

+  Plugin plugin;
+  std::string errmsg;
+  int code = plugin_func(&plugin, &errmsg);
+  if (code != 0) {  // TODO: Perhaps use different codes?


Like I said before, why not use Status? I don't like creating new public APIs with known "TODO" items calling for breaking changes

If we were ever to support dynamic plugins, Status could break ABI.

Additionally, some of the other functions in the ObjectRegistry (like NewFactoryObject) follow the same model (of returning something and an error string.

It would be possible to use the Status::Code enums as return codes (hence the TODO) to have this method return different Status for different cases (instead of always returning InvalidArgument).

If we were ever to support dynamic plugins, Status could break ABI.

What use is a RocksDB plugin that is ABI-incompatible with the compiled RocksDB?

A Plugin that is not ABI-compatible is not good, which is why there used to be code in the Plugin that tried to detect that condition and prevented it from loading. If a Plugin was not ABI-compatible and returned a Status here that was not ABI-compatible, it could cause the program to crash and would not be easy to debug or decipher the problem.

pdillinger · 2021-11-29T18:59:25Z

include/rocksdb/utilities/object_registry.h

+// The signature of the function for loading plugins
+// Plugin contains the struct of properties associated with this Plugin.
+// Plugin is passed to the PluginFunc and the fields should be initalized
+// by the Plugin code.


Plugin doesn't have code, only data. Perhaps "plugin code."

pdillinger · 2021-11-29T19:18:41Z

include/rocksdb/utilities/object_registry.h

+// return non-zero and set the std::string* to an appropriate error message.
+using PluginFunc = std::function<int(Plugin*, std::string*)>;
+
+struct Plugin {


What problem does this solve that is not solved by std::function<int(ObjectLibrary&)>? (Noting that passing non-const ref violates Google C++ guidelines)

There are a few reasons for the Plugin (as opposed to a simple RegistrarFunc):

If we ever intend to do error checking or provide more information about a plugin, we need some place to hold it. The Plugin struct was intended to be that place

The Registrar Func takes an argument. I am not sure where I am supposed to get that from (or just pass in "")

Without this struct, how could one determine what was loaded or available to the application? Using the Plugins and ObjectRegistry, we could add some useful information to tools to say what they are compiled with (e.g. this version of db_bench supports HDFS env). Without the Plugin struct/name, We would not have access to this information.

Also, this does not violate the Google C++ guidelines: "while non-optional output and input/output parameters should usually be references (which cannot be null)"

pdillinger · 2021-11-29T19:28:20Z

utilities/object_registry.cc

@@ -185,5 +207,29 @@ void ObjectRegistry::Dump(Logger *logger) const {
  }
 }

+Status ObjectRegistry::RegisterPlugin(const std::string &name,
+                                      const PluginFunc &plugin_func) {
+  Plugin plugin;


If we have a public type called "Plugin" then the plugin itself should control creation of the "Plugin" object, right? That's not what we have here. Best I can tell, here the object you are calling "Plugin" is a holder for a glorified callback for the ObjectRegistry. And we have a callback for populating the holder of the glorified callback. Why so many layers?

At the moment, the Plugin contains the name of the plugin, its registrar function, and an argument field. As I mentioned earlier, the struct previously contained more fields.

I could also envision the PluginFunc doing additional work. For example, a plugin that supports OpenSSL encryption may load the OpenSSL library dynamically and make sure the functions are available. If they are not, it might return an appropriate error message and not initialize registration functions. Or it might still register functions that return an error if the factory method is called.

Using just the ObjectRegistry call, there would be no way to the caller that Open SSL is not supported, except via the Factory method itself.

the Plugin contains the name of the plugin

Not currently

Right, it did and then I removed it as a duplication (there is also a name to the other methods).

Simpler implementation using RegisterPlugin(name, RegistrarFunc)

The Make processes were updated to support the new signature for RegisterPlugin. Both also support old-style syntax where <plugin>_FUNC is not defined

facebook-github-bot · 2022-01-10T22:08:41Z

@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-02-11T15:47:50Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

pdillinger

I think this is appropriately simple and effective. AFAIK, if plugins want to check rocksdb version at run time, they can do that with GetRocksVersionAsString (though that doesn't provide for easy comparison; maybe we should provide a variant of the version string for which lexicographic ordering agrees with version ordering).

pdillinger · 2022-02-11T16:39:55Z

CMakeLists.txt

+list(APPEND PLUGINS ${ROCKSDB_PLUGINS})
+foreach(PLUGIN IN LISTS PLUGINS)
+  set(PLUGIN_ROOT "${CMAKE_SOURCE_DIR}/plugin/${PLUGIN}/")
+  message("including rocksb plugin ${PLUGIN_ROOT}")


minor typo "rocksb"

pdillinger · 2022-02-11T16:54:05Z

include/rocksdb/utilities/object_registry.h

@@ -546,6 +548,8 @@ class ObjectRegistry {
  // The libraries are searched in reverse order (back to front) when
  // searching for entries.
  std::vector<std::shared_ptr<ObjectLibrary>> libraries_;
+  std::vector<std::string> plugins_;
+  static std::unordered_map<std::string, RegistrarFunc> builtins_;


I would prefer if the name of this field had a hint that it is static, such as static_builtins_.

pdillinger · 2022-02-11T17:09:27Z

Be sure to fix CI errors

facebook-github-bot · 2022-02-11T19:46:19Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-18T12:15:55Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-18T14:34:56Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-03-07T23:03:59Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-04-02T05:10:12Z

@mrambacher has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-04-02T05:14:42Z

@mrambacher has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mrambacher requested a review from ajkr February 9, 2021 23:00

facebook-github-bot added the CLA Signed label Feb 9, 2021

ajkr removed their request for review February 10, 2021 00:14

mrambacher mentioned this pull request Mar 2, 2021

Extensions to the ObjectRegistry #6591

Closed

mrambacher force-pushed the PluginRegistry branch 2 times, most recently from 8a0276a to f726f47 Compare June 9, 2021 19:46

mrambacher marked this pull request as ready for review June 9, 2021 20:01

mrambacher force-pushed the PluginRegistry branch from f27082d to 10439af Compare June 9, 2021 20:36

mrambacher force-pushed the PluginRegistry branch from 10439af to 018606f Compare June 9, 2021 22:16

mrambacher added 2 commits September 7, 2021 10:07

Update to latest

53bbcf1

Fix merge

0a4822b

mrambacher requested review from pdillinger and ajkr September 7, 2021 14:19

mrambacher added 11 commits September 7, 2021 11:03

Cleanup some of the Plugin code

50ac205

Rename PluginProperties to Plugin

d344f24

Merge branch 'main' into PluginRegistry

bd059aa

Merge branch 'main' into PluginRegistry

bdce7a5

Merge branch 'main' into PluginRegistry

0a7e15f

Update to latest

275ccac

Merge branch 'main' into PluginRegistry

ddfda52

Fix accidental checkin

4f53b99

Another format...

1000831

Merge branch 'main' into PluginRegistry

64bbde9

Merge branch 'main' into PluginRegistry

ca17c37

pdillinger requested changes Nov 15, 2021

View reviewed changes

Address PR Comments

9088324

riversand963 mentioned this pull request Nov 16, 2021

[RFC] Move HDFS support to separate repo #9170

Closed

Remove version and update comment.

a78850f

mrambacher requested a review from riversand963 November 25, 2021 11:35

pdillinger reviewed Nov 29, 2021

View reviewed changes

mrambacher added 3 commits November 29, 2021 16:55

Address some PR comments

7874f2a

Remove Plugin and PluginFunc

df11083

Simpler implementation using RegisterPlugin(name, RegistrarFunc)

Fix CMake and Make to support plugin builtins and externs

b57d4c2

The Make processes were updated to support the new signature for RegisterPlugin. Both also support old-style syntax where <plugin>_FUNC is not defined

mrambacher added 3 commits January 27, 2022 19:37

Merge branch 'main' into PluginRegistry

4159c0f

Merge to main

cf96b65

Merge branch 'main' into PluginRegistry

58b95e7

pdillinger approved these changes Feb 11, 2022

View reviewed changes

Fix merge issue

8fbdde4

Merge branch 'main' into PluginRegistry

9689578

Fix merge issue

9d77a1d

Merge branch 'main' into PluginRegistry

926fd18

Merge branch 'main' into PluginRegistry

2304d92

facebook-github-bot closed this in b7db7ea Apr 11, 2022

This was referenced Jun 21, 2022

Upgrade to rocksdb v7.3.1 rust-rocksdb/rust-rocksdb#645

Merged

Add batched version MultiGet API rust-rocksdb/rust-rocksdb#633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin Registry #7949

Plugin Registry #7949

mrambacher commented Feb 9, 2021

ajkr commented Feb 10, 2021

yhr commented Feb 11, 2021

mrambacher commented Jun 9, 2021

pdillinger Nov 15, 2021

mrambacher Nov 15, 2021

pdillinger left a comment

pdillinger Nov 16, 2021

pdillinger Nov 16, 2021

mrambacher Nov 29, 2021

pdillinger Nov 29, 2021

mrambacher Nov 29, 2021

pdillinger Nov 29, 2021

mrambacher Nov 29, 2021

pdillinger Nov 29, 2021

pdillinger Nov 29, 2021

mrambacher Nov 29, 2021

mrambacher Nov 29, 2021

pdillinger Nov 29, 2021

mrambacher Nov 29, 2021

pdillinger Nov 29, 2021

mrambacher Nov 29, 2021

facebook-github-bot commented Jan 10, 2022

facebook-github-bot commented Feb 11, 2022

pdillinger left a comment

pdillinger Feb 11, 2022

pdillinger Feb 11, 2022

pdillinger commented Feb 11, 2022

facebook-github-bot commented Feb 11, 2022

facebook-github-bot commented Feb 18, 2022

facebook-github-bot commented Feb 18, 2022

facebook-github-bot commented Mar 7, 2022

facebook-github-bot commented Apr 2, 2022

facebook-github-bot commented Apr 2, 2022

Plugin Registry #7949

Plugin Registry #7949

Conversation

mrambacher commented Feb 9, 2021

ajkr commented Feb 10, 2021

yhr commented Feb 11, 2021

mrambacher commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 10, 2022

facebook-github-bot commented Feb 11, 2022

pdillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdillinger commented Feb 11, 2022

facebook-github-bot commented Feb 11, 2022

facebook-github-bot commented Feb 18, 2022

facebook-github-bot commented Feb 18, 2022

facebook-github-bot commented Mar 7, 2022

facebook-github-bot commented Apr 2, 2022

facebook-github-bot commented Apr 2, 2022