[GitHub] [drill] paul-rogers commented on a change in pull request #2251: DRILL-7871 StoragePluginStore instances for different users

[GitBox <gi...@apache.org>]

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: dev-unsubscribe@drill.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org