paul-rogers commented on a change in pull request #2251:
URL: https://github.com/apache/drill/pull/2251#discussion_r662667413
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/StoragePluginRegistry.java
##########
@@ -21,19 +21,18 @@
import java.util.Map;
import java.util.Set;
+import org.apache.calcite.schema.SchemaPlus;
Review comment:
The plugin registry is a complex beast. Before this change, it supported
built-in plugins and those provided in the plugins folder. The registry handles
both the "connector" (code) and "instances" (code + a config.)
This change adds per-user plugins. The overall bug description does not seem
to provide answers to several questions. For example:
* Are we controlling access to the "connectors" (code) or to the "instances"
(configs?)
* If we control the connectors, do we load new copies of the connector
metadata per user?
* Can a user see the common set of system-wide configs? Or, only their own
private configs?
* If system wide, how do we handle name conflicts? I add a "foo" plugin,
later the admin adds a system-wide "foo" or visa-versa.
* Can a private plugin be promoted to system level? Is that a copy/paste
operation? With the user logged in as different windows? Or, is there a button
to do the operation?
* When is the per-user information created? As you know, on the web, the
user session comes into existence for every single REST call. Plugin setup is
pretty heavy-weight. Is the user config cached between requests? If so, when do
we expire the user information if the user then becomes idle?
* The registry handles updates. The current code is rather awful since there
is no coordination among Drillbits: multiple drillbits could come up at the
same time and they can all decide to do the upgrade. Adding users complicates
the effort. Will a user session trigger a plugin upgrade? How do we handle
races?
* This registry handles a subtle, obscure case. Suppose (before this
change), I run a query that users the "foo" plugin. Before the query starts
running (but after planning), I rush in and delete the "foo" config. What
happens? As it turns out, Drill saves the old config so that queries continue
to work. This works because there is only one registry. If we add per-user
registries, we'd need to pass the user info with the exec plan so that the
readers grab the user's version of the config, including any cached,
recently-deleted configs. (Distributed systems are complex!)
* There are probably a few more issues that also deserve consideration. The
comments in this part of the code try to explain the various issues.
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/work/prepare/PreparedStatementProvider.java
##########
@@ -138,7 +139,9 @@ public void run() {
final QueryId limit0QueryId = userWorker.submitWork(wrapper,
limit0Query);
final long timeoutMillis =
-
userWorker.getSystemOptions().getOption(CREATE_PREPARE_STATEMENT_TIMEOUT_MILLIS).num_val;
+
userWorker.getDrillbitContext().getConfig().getBoolean(ExecConstants.SEPARATE_WORKSPACE)
Review comment:
As noted above, this kind of code will lead to countless subtle errors.
Suggestion: introduce yet another option manager whose job is to decide which
of the two "system" option managers is needed. Option managers are designed to
nest, so this should be not too hard.
And, there is still the question: why do we need two *system* option
managers? Do users get their own system options now? See questions above...
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StatusResources.java
##########
@@ -183,9 +188,9 @@ public Viewable getSystemInternalOptions(@Context UriInfo
uriInfo) {
public Viewable updateSystemOption(@FormParam("name") String name,
@FormParam("value") String value,
@FormParam("kind") String kind) {
- SystemOptionManager optionManager = work.getContext()
- .getOptionManager();
-
+ SystemOptionManager optionManager = authEnabled.separateWorkspace()
Review comment:
This seems to say that updating a *system* option works differently
depending on whether the user has their own workspace. Shouldn't *system*
options always update the (global) system options? Otherwise, do we need a
"real system option" attribute to really update the global options?
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StatusResources.java
##########
@@ -112,7 +115,9 @@ public String getMetrics(@PathParam("hostname") String
hostname) throws Exceptio
private List<OptionWrapper> getSystemOptionsJSONHelper(boolean internal)
{
List<OptionWrapper> options = new LinkedList<>();
- OptionManager optionManager = work.getContext().getOptionManager();
+ OptionManager optionManager = authEnabled.separateWorkspace()
Review comment:
This is a complex bit of code which is repeated several times. Can it be
factored out somewhere?
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/StorageResources.java
##########
@@ -138,7 +141,9 @@ public Viewable getPlugins() {
@Produces(MediaType.APPLICATION_JSON)
public PluginConfigWrapper getPluginConfig(@PathParam("name") String name) {
try {
- return new PluginConfigWrapper(name, storage.getStoredConfig(name));
+ return authEnabled.separateWorkspace()
Review comment:
On this, and similar lines, it would be better to factor out the logic
into a method on `WebUserConnection` such as `getStorage()` or some such. Too
much copy/paste and room for error otherwise.
##########
File path:
exec/java-exec/src/test/java/org/apache/drill/exec/store/TestUserPluginRegistry.java
##########
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.exec.store;
+
+import org.apache.drill.common.config.DrillProperties;
+import org.apache.drill.common.logical.StoragePluginConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.rpc.user.UserServer;
+import org.apache.drill.exec.rpc.user.UserSession;
+import
org.apache.drill.exec.rpc.user.security.testing.UserAuthenticatorTestImpl;
+import org.apache.drill.exec.store.dfs.FileSystemConfig;
+import org.apache.drill.exec.store.dfs.FileSystemPlugin;
+import org.apache.drill.test.ClientFixture;
+import org.apache.drill.test.ClusterFixture;
+import org.apache.drill.test.ClusterFixtureBuilder;
+import org.junit.Test;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static
org.apache.drill.exec.rpc.user.security.testing.UserAuthenticatorTestImpl.TEST_USER_1;
+import static
org.apache.drill.exec.rpc.user.security.testing.UserAuthenticatorTestImpl.TEST_USER_1_PASSWORD;
+import static
org.apache.drill.exec.rpc.user.security.testing.UserAuthenticatorTestImpl.TEST_USER_2;
+import static
org.apache.drill.exec.rpc.user.security.testing.UserAuthenticatorTestImpl.TEST_USER_2_PASSWORD;
+import static org.apache.drill.exec.util.StoragePluginTestUtils.CP_PLUGIN_NAME;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+public class TestUserPluginRegistry extends BasePluginRegistry {
Review comment:
Thanks for providing these tests. Thanks for building on the tests added
back when I refactored the plugin registry: there were many gaps in test
coverage before that time.
Do the test also test interaction of the per-user registry with the shared
system registry? See the above comment for the kinds of things to test:
duplicate names, etc.
This area is highly complex (unfortunately) so we have to be very careful so
we don't break stuff.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
