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

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

vdiravka commented on a change in pull request #2251:
URL: https://github.com/apache/drill/pull/2251#discussion_r663159582



##########
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:
       Done.

##########
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.
   
   - Yes
   
   > 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?
   
   - No, we don't care about real system options in case separate_workspace is enabled. The user and all his activity relies on his own SystemOptions.

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
##########
@@ -71,6 +73,87 @@
  */
 public class SystemOptionManager extends BaseOptionManager implements AutoCloseable {
   private static final Logger logger = LoggerFactory.getLogger(SystemOptionManager.class);
+  public static final String OPTIONS_PSTORE_NAME = "options";

Review comment:
       _Note:_ changes in `SystemOptionManager` is just refactoring (placing code blocks in right places)

##########
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:
       The per-user registry doesn't interact with the system one. Except that every per-user one has an initial state corresponding to the system one.
   
   Also we may want in future to enable this feature by default and then check all tests pass.
   

##########
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:
       fully agree. Done

##########
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:
       Yes, every user starts to have the separate instance of `SystemOptions`. The original one is needed for consistency and to start Drillbit. We can chose in future one of the ways: 
   * removing the global SystemOptions 
   * leveraging this global SystemOptions for some purpose (as describe in 5.2 section of [doc](https://docs.google.com/document/d/1mFjDGPHID70SG5uwP8FZImI0eyEwjSmbTCaSzOU7sjc/edit#heading=h.iqtfxhbbot9c))
   




-- 
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