paul-rogers commented on issue #2010: DRILL-7620: Fix plugin mutability issues URL: https://github.com/apache/drill/pull/2010#issuecomment-597381178 @agozhiy, here is how I read the old behavior: * The UI sends the plugin as JSON via the `createOrUpdatePlugin()` mehod. * `StorageResources` configures its mapper (which is really the global one from `LogicalPlanPersistence`) to accept comments, then deserializes the JSON to create a config. * If deserialization fails, the UI returns an error message. * `StorageResources` passes the name and config, as a `PluginConfigWrapper` to `createOrUpdatePluginJSON()` which calls `createOrUpdate()` in the plugin registry. * `createOrUpdate()` first creates an instance of the plugin. The result will be one of a) `null` if the config is disabled, b) a `StoragePlugin` if enabled and valid, or c) an exception if invalid. * `createOrUpdate()` now does one of: * If enabled, replace existing plugin in the in-memory cache with the new one, or add the new one * If disabled, remove the existing plugin from the in-memory cache * `createOrUpdate()` then writes the config to persistent storage independent of the above choices. There are bugs in the above. The above writes to the local cache before storage meaning that, for a brief time, the two are out of sync. If, during that time, another update comes in, then the two will again be out of sync. These issues don't crop up in the current tests, of course. The new code does: * The UI sends the plugin as JSON via the `createOrUpdatePlugin()` mehod. * `StorageResources` calls `putJson()` in the plugin registry with the name and JSON text. * `putJson()` decodes the JSON using the system mapper, configured to allow comments, throwing an exception if the decode fails. If succeeds, calls `put()` with the config. * `put()` validates that the plugin is not a system plugin, then updates persistent storage. I see the problem, the `put()` call above should be the new `validatedPut()` which creates an instance of the plugin first. Once we make that fix: * `validatedPut()` creates an instance of the plugin, throwing an exception if instance creation fails. * If OK, `validatedPut()` writes he plugin to persistent storage and updates the plugin cache. There is a second bug here: the logic should be: * `validatedPut()` does one of: * If enabled, updates the config in the plugin cache. * If disabled, removes the config from the plugin cache. * `validatedPut()` then checks if the old entry had a plugin instance. If , adds it to the ephemeral store. * `validatedPut()` writes the updated config to persistent storage. The planner bugs explained earlier still occur. (A query should not iterate over all registered plugins.) Still, the above fixes should get us back to the original behavior, with the addition of the other fixes we're trying to make. Made the two fixes noted above. Added unit tests for this case. @agozhiy, if you run the UI scenario from your earlier comment, you should see that the plugin is rejected if invalid and enabled. The config is stored, but not instantiated, if invalid and disabled. If you are brave, you can use ZK to change a stored valid config to make it invalid. Doing so simulates the external system failing. With the old version, Drill will fail on startup. With this version, Drill will start, but you'll see error messages in the log each time you plan a query. A future PR will ensure you see no errors at all. All of this points out more open issues: * If a plugin ever fails, should it be disabled? How would this work for a temporary outage? The user would have to manually re-enable the plugin when it becomes valid again. * On a completely unrelated topic, there is code in the original version that forces all names of stored plugins to lower case (removing any mixed-case versions and replacing them), but no code enforces the lower-case rule in the in-memory cache. (I have tests that specifically test this case.) However, names are forced to lower case in storage. This seems more on the "bug" side of the ledger than the "feature" side. Should we allow mixed case everywhere or nowhere? Yet another thing to fix in the next PR.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
