-
Notifications
You must be signed in to change notification settings - Fork 982
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
DRILL-7590: Refactor plugin registry #1988
Conversation
Java 13 build failed due to known problems:
Java 8 build seemed very slow. For example:
Things were not that slow on my dev box. Is this an issue with the GitHub build? |
I think the difference in time execution of tests is caused by the difference of resources for GithubActions env and dev box. |
@vvysotskyi , most tests now have category tags. Should we use categories to select a set of "smoke test" case to run via GitHub? We could exclude "slow" and "unlikely" tests for example. Otherwise, it would be an effort to identify the core set of "smoke tests." Every PR is different. One thought is to run a set of high-level tests (i.e. queries) that exercise the whole system. If something fails (such as if this PR broke some aspect of plugin registration), the smoke tests won't tell us the specific source of failure. However, if the smoke tests fail, the developer can rerun the full unit test suite to zero in on the specific problem. That way, if this particular test takes a long time, but exercises much of Drill, it would be a good candidate for a smoke test. We'd make the time by removing many of the detailed, focused unit tests (such as those created as part of this PR.) You've probably thought about solutions. What do you recommend? |
The problem is that not all contributors run full tests suite before opening the pull request or rerunning it after making some changes. So if we will exclude some tests from GitHub Actions, we would increase cases when some failing tests were found only when doing batch commits, so batch committer would have to find PR which caused the failures, point contributor to the failing test and after that reviewer should do the review again. I like the approach of running focused unit tests for narrowing down the problem, and as you pointed, it may be reached by using existing unit tests categories. I think we should at least document in contribution guide that the way how slow and unlikely tests may be excluded from the tests run in the case of issues to find a simpler test to reproduce the issue.
|
@vvysotskyi, the ideal solution is to make the full "pre-commit" test suite available to committers. That way, for example, I could help with the commit process as I used to do when at MapR. The workflow would then be:
Drill used to do batch commits on Friday. This meant that some engineer had to do the merges and run the pre-commit tests. Took time. In Impala, we each ran the equivalent of the pre-commit tests and merged our own patches after approval. Since this could be done offline (run in the evening, say), it was not as much of a time sink as the old Drill process. I suppose there are two questions. 1) Do you (or someone on the team) understand the current system well enough to set up a duplicate on, say, AWS? and, of course, 2) Where do we get funding for the AWS (or GCE) instance? |
logical/src/main/java/org/apache/drill/common/logical/data/LogicalOperatorBase.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/apache/drill/common/PlanStringBuilder.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java
Outdated
Show resolved
Hide resolved
contrib/format-excel/src/main/java/org/apache/drill/exec/store/excel/ExcelFormatConfig.java
Outdated
Show resolved
Hide resolved
} catch (UserException e) { | ||
throw e; | ||
} catch (Exception e) { | ||
plugin = null; |
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.
Why we only here make plugin null? Why not for UserException as well?
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.
I assumed that the user exception would cause the whole operation to fail so that the handle itself would be discarded. Still, added code to set it null to make the code less confusing.
* Atomically transfer the plugin instance, if any, to a new handle | ||
* of the given type. | ||
*/ | ||
public synchronized PluginHandle transfer(PluginType type) { |
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.
Not quite sure what his method does and what for it is used...
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.
This handles a strange case, updated the comments. Better would be to combine the ephemeral and stored plugin caches and synchronize the combined set. But, didn't want to mess with creating a time out thread in this PR, so left them unsynchronized except via this transfer process.
"%s -> %s and %s -> %s", | ||
connector.configClass().getName(), connector.locator().getClass().getName(), | ||
prev.configClass().getName(), prev.locator().getClass().getName()); | ||
logger.error(msg); |
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.
Usually we should do only one either log or fail.
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.
Agreed. I think this pattern was original (except that the message was repeated.) From personal experience, this kind of error is hard to track down, so I wanted to log as much info as possible. But, the error indicates that the bootstrap plugin file, or plugin code, has bugs, so we need to shut down.
We need a non-user exception that logs and throws like User Exception, but for times like this.
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
Outdated
Show resolved
Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistryImpl.java
Outdated
Show resolved
Hide resolved
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.
@arina-ielchiieva, thanks much for taking the time to review this large PR. Have addressed your comments.
When rerunning unit tests, I'm now experiencing failures in TestDynamicUDFSupport
. I'll investigate tomorrow to figure out what might have broken. That failure prevented running tests past java-exec
, so I'll finish those once I track down the blocking issue.
@@ -65,6 +65,7 @@ public static void setup() throws Exception { | |||
|
|||
ExcelFormatConfig formatConfig = new ExcelFormatConfig(); | |||
cluster.defineFormat("cp", "excel", formatConfig); | |||
cluster.defineFormat("dfs", "excel", formatConfig); |
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.
The tests failed without it. Don't know why. I suspect this is a result of being more strict about not mucking with configs after they are stored in the plugin registry. That is, the prior behavior was wrong, but benign. This change is "more right."
this(new HashMap<>()); | ||
} | ||
|
||
// TODO: Move to a test |
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.
Tried to move to a test. Code does not work. Functionality itself tested indirectly in new registry unit tests, so just deleted the method.
* @return A map | ||
*/ | ||
public static CaseInsensitiveMap<OptionDefinition> createDefaultOptionDefinitions() { | ||
@SuppressWarnings("deprecation") |
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.
Here, yes. The deprecation says not to use the option in code. But, for backward compatibility, we need to keep the old options in the table to avoid failures if users reference the options. Added comment.
|
||
@Override | ||
public void close() { | ||
this.close(); |
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.
Oh my. Good catch.
* provide a version number. | ||
*/ | ||
public interface PluginBootstrapLoader { | ||
StoragePlugins bootstrapPlugins() throws IOException; |
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.
I went back and forth. In the end, I decided that the loader should just focus on reporting its own errors, then let the registry provide the user exception context.
@Override | ||
public StoragePlugins updatedPlugins() { | ||
// ... and no upgrades | ||
return null; |
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.
Same answer.
} | ||
|
||
@Override | ||
public void onUpgrade() { } |
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.
This says that an upgrade happened. No harm in doing nothing to show that this locator is not interested in upgrades.
@Test | ||
public void testClassList() throws Exception { | ||
OperatorFixture.Builder builder = OperatorFixture.builder(dirTestWatcher); | ||
// builder.configBuilder().put(ClassPathScanner.IMPLEMENTATIONS_SCAN_CACHE, false); |
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.
Added a comment. I spent far too much time tracking down why the tests failed randomly before I realized the cached class path was screwing me up. So, left these in as a reminder for the next time I (or someone else) is in the same situation.
public class TestSystemPluginLocator extends BasePluginRegistryTest { | ||
|
||
// Uses a null Drillbit context. This is benign for the current | ||
// set of system plugins. If this test fails, check if a system |
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.
This is the only way to test the system plugins at this level. However, I didn't want to start an entire Drillbit just to have a context which is never used. Felt wrong to test low-level code with a dependency on the entire product.
As the comment says, the next PR will change the context passed to plugins to a smaller interface (only methods actually used) so it can be created (or mocked) here. At that point, this test will become safe. So, the current code is temporary.
private Boolean enabled; | ||
|
||
public StoragePluginConfig() { | ||
enabled = true; |
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.
Removed. I'll fix this in the next PR. IMHO, plugins should start enabled, and should become disabled only via config. (This is fine, because the only three ways we create configs is 1) from the bootstrap plugins file, 2) user copy & paste, and 3) in test code.)
The flag can be a primitive boolean
as long as constructors accept a Boolean
.
The focus in this PR is on the registry; I (mostly) resisted the urge to fix the plugin API in this PR, so I'll back out this change also.
public PlanStringBuilder field(String key, int value) { | ||
startField(key); | ||
buf.append(value); | ||
return this; | ||
} | ||
|
||
/** | ||
* Displays a character in Java-quoted format: {@code delimiter="\n"}. | ||
* @param key |
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.
Please add descriptions, sorry for asking that, it's just IDE warnings are annoying when descriptions are not missing.
* Construct a handle for a "normal" connector which takes a plugin config | ||
* and constructs a plugin instance based on that config. | ||
*/ | ||
public static ConnectorHandle configuredConnector(ConnectorLocator locator, |
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 adding static methods, now it looks really nice!
@paul-rogers thanks for making changes and addressing code review comments. Regarding |
@arina-ielchiieva, am working on the UDF test failure. Looks like the recent change to move the UDF |
91147b9
to
979ea19
Compare
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.
@arina-ielchiieva thanks for catching the format bootstrap issue. It turns out that we have only one unit test for this functionality: the Excel plugin which assumes that bootstrap plugins work for dfs
, but not cp
. Made a note to fix this in the next PR: look for unit tests that add plugins manually and check if they should instead test the format bootstrap file.
Please let me know if there is anything else we should do on this PR.
@@ -65,6 +65,7 @@ public static void setup() throws Exception { | |||
|
|||
ExcelFormatConfig formatConfig = new ExcelFormatConfig(); | |||
cluster.defineFormat("cp", "excel", formatConfig); | |||
cluster.defineFormat("dfs", "excel", formatConfig); |
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.
I should have realized this was telling me something. Was due to my misunderstanding of how format plugin bootstrap works. Reverted change. And, added Excel to cp to avoid the need for the "cp" setup.
logger.info("No storage plugin instances configured in persistent store, loading bootstrap configuration."); | ||
String storageBootstrapFileName = context.config().getString(ExecConstants.BOOTSTRAP_STORAGE_PLUGINS_FILE); | ||
Set<URL> storageUrls = ClassPathScanner.forResource(storageBootstrapFileName, false); | ||
// Format plugins does not work, see note below. |
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.
Restored the format bootstrap functionality. At present, it will only install formats at bootstrap time, it won't add new formats to existing configs which, it seems, is how it worked in Drill 1.17. Also added a unit test.
@paul-rogers changes look good to me. I was going to commit them but found one problem. Disabled plugins are not displayed on Web UI. Though if you check Drill folder you can see they are there:
Please fix, squash the commits and rebase to the latest master. |
@arina-ielchiieva, thanks for checking. Annoying that we don't have unit tests for this. I'll take a look and find a fix, then squash commits. |
c92a913
to
b5a4ddf
Compare
@arina-ielchiieva, thanks again for your thorough review. Fixed the plugin bootstrap issue. Added unit tests. Also verified manually using the UI. Since I had to rerun all tests, went ahead and modified format plugin tests to rely on the bootstrap format config rather than adding format plugins manually. Also fixed a nagging issue: that the registry loaded plugins twice on bootstrap. Adjusted the code so it happens only once. We use the bootstrap files if the persistent store load comes up empty. Squashed all but the above commit so it is easy to see these final changes. |
exec/java-exec/src/test/java/org/apache/drill/exec/store/TestPluginRegistry.java
Outdated
Show resolved
Hide resolved
@paul-rogers please address minor spelling issue and squash the commits, than I'll run the tests and commit. |
Major cleanup of the plugin registry to split it into components in preparation for a proper plugin API. Better coordinates the named and ephemeral plugin caches. Cleans up the registry API. Sharpens rules for modifying plugin configs.
@arina-ielchiieva, thanks for catching the type. Now fixed. Squashed commits. Thanks for doing the full pre-commit test run: those tests use Drill closer to "real production" than unit tests and will tell us if anything else still needs fixing. |
@paul-rogers found one more issue but unfortunately after I have merged the PR. |
Performs a thorough "spring cleaning" of the storage plugin registry to prepare it to add a proper plugin API.
This is a complex PR with lots going on.
The plugin registry connects configurations, stored in ZK, with implementations, which are Java classes. The registry handles a large number of tasks including:
The code has grown overly complex. As we look to add a new, cleaner plugin mechanism, we will start by cleaning up what we have to allow the new mechanism to be one of many.
Terminology
There is no more confusing term in Drill than "plugin." That single term can mean:
of a plugin.
To make the following discussion clearer, we redefine terms as:
of a connector + config pair.
Standard and System Connectors
The registry class handled many tasks itself, making the code hard to follow. The first task is to split apart responsibilities into separate classses.
The registry handles two kinds of plugins at present:
StoragePluginConfig
subclass and aStoragePlugin
subclasswith a specific constructor. Their configs are persistently stored in ZK. That is, the storage plugins most of us think about.
Examples:
sys
andinformation_schema
System plugins have the `` annotation, are created at boot time, and do not reside inthe ZK store.
The first step is to split out these two kinds of plugins into separate "provider" classes, along with a common interface. A new
ConnectorProvider
interface has two implementations: one for "classic" plugins another for system plugins. Then, when we add the new mechanism, it becomes a third plugin provider.Bootstrap and Upgrade
The registry also handles the process of initializing a newly installed Drill, or upgrading an existing one. The code for this is pulled out into a separate class.
Moved the names of the bootstrap plugins and plugins upgrade files into the config system
to allow easier testing with test-specific files. Added complete unit tests.
The registry used to load plugins twice on startup: once to see if any exist, another to populate the internal cache. Change this to a single load.
Plugin Lifecycle
Plugins have a surprisingly robust lifecycle. Revised the code to better model the nuances of the
lifecycle (and fix a number of subtle bugs).
Plugin instances must be created, but only for standard plugins (not system plugins). Added a
ConnectorHandle
so we can track the source of each connector so that the locator can createconnector instances (for standard plugins) or not (for system plugins.)
Plugins are defined by persistent storage as a (name, config) pair. There is no reason to
create a connector instance just to load plugins from storage. So, added a
PluginHandle
classto hold onto the (name, config,
ConnectorHandle
) triple.This handle then allows us to do lazy instantiation of the connector class. Rather than creating
it on load, we wait until some code actually needs the plugin. (Some code still demands that we load all plugins; this can be fixed in a later PR.)
The registry API was changed to make this clear.
createOrUpdate()
is renamed toput
andno longer returns the plugin instance (which, it turned out, was never used.) Now, we don't
create the connector instance until
getPlugin()
is called. Added a newgetConfig()
method for the many times we only want the config and don't actually need the instance.Drill is a concurrent, distributed system. Plugin (configurations) can change at any time.
We might change
dfs
while queries run. The registry supports "ephemeral" plugins, thosethat occur in a query execution plan, but do not match a name in persistent storage.
Previously, ephemeral plugins were not connected to normal named plugins. Revised this so that
plugin instances migrate from the named cache to the ephemeral cache and back again as the
user changes plugin definitions. This avoids having two plugins with the same configuration
active at the same time (unless those plugins have different names.) It also prevents redundant close/open pairs on the plugin instance.
Plugin Bootstrap
The registry provides a way to load plugins on first start, and to upgrade plugins
in existing versions. This code was gathered into a
PluginBootstrapLoader
interfaceand implementation so it can be extended for the new plugin system.
The recent feature to bootstrap format plugins is a bit awkward, but was retained when moving code around. Unit tests added. Modified format plugin unit tests to rely on the bootstrap formats which now exist.
The upgrade code seems to have never been used, and would be somewhat fragile if we
tried. It relies on detecting that a certain file exists in Drill, doing the upgrade
if do, then deleting the file. While this works in the ideal case, it does not work if
the same Drill install is used with two persistent stores, if the persistent store is
restored to an earlier state, or if the user upgrades across multiple versions in one
go. We need a version number in the persistent store in order to make upgrade work
well, but doing so is out of scope of this particular PR.
Concurrency
The
StoragePluginMap
maintains two maps: one from name to plugins, another from configto plugins. The map tried to be thread-safe, but had some race conditions that, under heavy load,
the two maps could get out of sync. Reworked this class to avoid the race conditions. Then,
carefully reviewed all code paths to ensure that either we obtain locks (for the
StoragePluginMap
)or the code flow is such that a race condition is benign.
There was some unfortunate code that modified storage plugin configs after they were stored in the registry. This must be strictly forbidden as it causes the (name --> plugin) and (config --> plugin) maps to get out of sync. Fixed all such issues that could be found.
Removed Ad-Hoc Plugins
The registry contained support for ad-hoc plugins: the ability to pass
false
for theisPersistent
argument to the oldcreateOrUpdate()
method. These were added (in part by this author)to support test-only plugins. However, ad-hoc plugins create very messy semantics. For example,
once we get the list of plugin classes at startup, we want the list to be immutable to avoid
the need to synchronize the list. Adding test-only classes after start breaks this assumption.
Test-only plugin configs can just be added with the
put()
method. Such plugins will persist in the plugin store. This is fine, because tests usually use a throw-away persistent store after each test.Custom plugin classes can be marked with the new
@PrivatePlugin
annotation so that the"classic" locator won't load them. Then, set the new
PRIVATE_CONNECTORS
property to listthose private classes that should be loaded. The private plugins are then available for use.
Revise Registry API
The registry wants to be pluggable (though we have only one implementation). Replaced the method to get the underlying store with one to return the list of plugins, which is what the only client of the former method needed.
Already mentioned that the
createOrUpdate
method changed to the simplerput
. Added a method to get a config without the plugin. Removed the plugin return fromput
since no codeused it and it forces creation of the plugin (see below).
Advice for Test Creators
The right way to create a plugin for a unit test is:
added the class, you may need to do a new build, or use the code shown below to force
a run-time scan.)
put
method on the registry giving a (name, config) pair.get()
method with the name.ClusterTest
, calldefineStoragePlugin()
to define the plugin on all Drillbits.The code to force a run-time class scan is:
Also, if you want to change the content of a plugin config stored in the registry, you
must make a copy first, else the internal maps will get out of sync. Think of formats
as immutable once stored in the registry. Code should be modified to reflect this fact.
Retire the "Named" Storage Plugin
Execution plans sent from the planner to the execution engine contain the full configuration, in JSON form, for any storage or format plugins used by the query. This may seem redundant: why include the information in the plan when we could obtain it from the plugin registry? All we need is the plugin name and we can do the retrieval.
The code contained an obscure plugin config classes:
NamedStoragePluginConfig
which seemed to implement this idea. At present, they are used by exactly one obscure test:TextRecordReaderTest
.One might think that we should broaden the use of this idea. But, a bit of thought suggests why the original authors might not have done so. Consider what happens if the user changes the config concurrently with running a query. The update and query "race" each other to the worker nodes.
In the worst case, some nodes run the query with the old configuration, some with the new, causing infrequent, random and obscure errors.
By copying the config into the plan, we avoid the race condition. It is a poor man's versioned plugin storage. (The ephemeral store ensures that the query sees the "old" config for the duration of the query.)
A more elegant solution might be to version each plugin configuration in the shared DB so that the "named" config could say, "I want the s3 plugin at version 2", even while version 3 is being propagated across the cluster.
However, since the current system works, and there are no plans for a versioned system, it seems to make sense to simply remove the unused "named" plugins and remove the one tests which uses them.
Note that the code also contains a
NamedFormatPluginConfig
class used in several places. This format allows forcing the type of a format in a query. This format does not have the same issues as the above class, and so remains in the code.Add Unit Tests
At present there are no unit tests that directly exercise the plugin registry. An analysis of a test run showed that some code is called only a few times, by tests that are doing something that only indirectly relates to plugins. (The previously-mentioned,
TextRecordReaderTest
, which was the only use of a specific if-statement, impersonation tests which are the only use of a "delete" method, and so on.Add a unit tests that directly tests all plugin functionality. Refactor the plugin code to allow such tests. (The upgrade mechanism, for example, only works with a very specific system state which is hard to reproduce in tests.)
Plugin Context
Plugin constructs accept a
DrillbitContext
instance.DrillbitContext
provides access to allof Drill's internals. As we think about an improved plugin API, it is clear we cannot have
external code depend on Drill's internals. We need a plugin-specific context. This PR takes
a step in that direction with the
PluginRegistryContext
interface used, at present, onlyby the registry itself. This class also allows mocking for unit tests.
A future PR will revise storage plugins to accept a new
StoragePluginContext
interface instead ofDrillbitContext
.Logical Plan Persistence
Cleaned up redundant code for finding serialized classes for operators. Cleaned up references to
LogicalPlanPersistence
whenObjectMapper
would do.Random Other Changes
DrillSchemaFactory
into a separate file.DistributedStorageEngine
,BatchExceededException
,StorageDriver
,StorageDriverFactory
, andPluginHost
classes.StoragePlugin
interface, but did not modify the set of methods.PlanStringBuilder
class and used it to addtoString()
methods to several plugins.Additional Work
A number of additional improvements are possible:
resolve schemas only as referenced.
between classic and system connectors locators.
StoragePlugin
class to accept a plugin-specific context instead ofthe "kitchen sink"
DrillbitContext
.Documentation
Care was taken to ensure that, despite the many code changes, there is no visible difference in behavior for end users.
Testing
Added multiple new unit tests. Reran all unit tests and fixed issues.