[jira] [Commented] (DRILL-4287) Do lazy reading of parquet metadata cache file

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4287?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148166#comment-15148166
 ] 

ASF GitHub Bot commented on DRILL-4287:
---

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/376#discussion_r52973451
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java
 ---
@@ -529,6 +549,36 @@ public long getRowCount() {
 
   }
 
+
+  // Create and return a new file selection based on reading the metadata 
cache file.
+  // This function also initializes a few of ParquetGroupScan's fields as 
appropriate.
+  private FileSelection
+  initFromMetadataCache(DrillFileSystem fs, FileSelection selection) 
throws IOException {
+FileStatus metaRootDir = selection.getFirstPath(fs);
+Path metaFilePath = new Path(metaRootDir.getPath(), 
Metadata.METADATA_FILENAME);
+
+// get (and set internal field) the metadata for the directory by 
reading the metadata file
+this.parquetTableMetadata = Metadata.readBlockMeta(fs, 
metaFilePath.toString());
+List fileNames = Lists.newArrayList();
+for (Metadata.ParquetFileMetadata file : 
parquetTableMetadata.getFiles()) {
+  fileNames.add(file.getPath());
+}
+// when creating the file selection, set the selection root in the 
form /a/b instead of
+// file:/a/b.  The reason is that the file names above have been 
created in the form
+// /a/b/c.parquet and the format of the selection root must match that 
of the file names
+// otherwise downstream operations such as partition pruning can break.
+final Path metaRootPath = 
Path.getPathWithoutSchemeAndAuthority(metaRootDir.getPath());
+this.selectionRoot = metaRootPath.toString();
+
+// Use the FileSelection constructor directly here instead of the 
FileSelection.create() method
+// because create() changes the root to include the scheme and 
authority; In future, if create()
+// is the preferred way to instantiate a file selection, we may need 
to do something different...
+FileSelection newSelection = new 
FileSelection(selection.getStatuses(fs), fileNames, metaRootPath.toString());
--- End diff --

Seems the first argument : selection.getStatuses(fs) is the file status 
before being expanded from cache, while the second argument fileNames 
represents the expanded file names from cache.  Will such difference cause any 
problem? 


> Do lazy reading of parquet metadata cache file
> --
>
> Key: DRILL-4287
> URL: https://issues.apache.org/jira/browse/DRILL-4287
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Affects Versions: 1.4.0
>Reporter: Aman Sinha
>Assignee: Jinfeng Ni
>
> Currently, the parquet metadata cache file is read eagerly during creation of 
> the DrillTable (as part of ParquetFormatMatcher.isReadable()).  This is not 
> desirable from performance standpoint since there are scenarios where we want 
> to do some up-front optimizations - e.g. directory-based partition pruning 
> (see DRILL-2517) or potential limit 0 optimization etc. - and in such 
> situations it is better to do lazy reading of the metadata cache file.   
> This is a placeholder to perform such delayed reading since it is needed for 
> the aforementioned optimizations.  



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4287) Do lazy reading of parquet metadata cache file

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4287?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148165#comment-15148165
 ] 

ASF GitHub Bot commented on DRILL-4287:
---

Github user jinfengni commented on a diff in the pull request:

https://github.com/apache/drill/pull/376#discussion_r52973315
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/FileSelection.java 
---
@@ -68,6 +79,7 @@ protected FileSelection(final FileSelection selection) {
 this.statuses = selection.statuses;
 this.files = selection.files;
 this.selectionRoot = selection.selectionRoot;
+this.dirStatus = new BitSet(StatusType.values().length);
--- End diff --

Should this constructor copy the dirStatus from the input "selection" as 
well?  


> Do lazy reading of parquet metadata cache file
> --
>
> Key: DRILL-4287
> URL: https://issues.apache.org/jira/browse/DRILL-4287
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Affects Versions: 1.4.0
>Reporter: Aman Sinha
>Assignee: Jinfeng Ni
>
> Currently, the parquet metadata cache file is read eagerly during creation of 
> the DrillTable (as part of ParquetFormatMatcher.isReadable()).  This is not 
> desirable from performance standpoint since there are scenarios where we want 
> to do some up-front optimizations - e.g. directory-based partition pruning 
> (see DRILL-2517) or potential limit 0 optimization etc. - and in such 
> situations it is better to do lazy reading of the metadata cache file.   
> This is a placeholder to perform such delayed reading since it is needed for 
> the aforementioned optimizations.  



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4287) Do lazy reading of parquet metadata cache file

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4287?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148024#comment-15148024
 ] 

ASF GitHub Bot commented on DRILL-4287:
---

Github user amansinha100 commented on the pull request:

https://github.com/apache/drill/pull/376#issuecomment-184481180
  
I have incorporated the review comments and updated the PR.  Please take a 
look when you get a chance.


> Do lazy reading of parquet metadata cache file
> --
>
> Key: DRILL-4287
> URL: https://issues.apache.org/jira/browse/DRILL-4287
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Query Planning & Optimization
>Affects Versions: 1.4.0
>Reporter: Aman Sinha
>Assignee: Jinfeng Ni
>
> Currently, the parquet metadata cache file is read eagerly during creation of 
> the DrillTable (as part of ParquetFormatMatcher.isReadable()).  This is not 
> desirable from performance standpoint since there are scenarios where we want 
> to do some up-front optimizations - e.g. directory-based partition pruning 
> (see DRILL-2517) or potential limit 0 optimization etc. - and in such 
> situations it is better to do lazy reading of the metadata cache file.   
> This is a placeholder to perform such delayed reading since it is needed for 
> the aforementioned optimizations.  



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148017#comment-15148017
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52962267
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/LocalPersistentStoreProvider.java
 ---
@@ -0,0 +1,74 @@
+/**
+ * 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.sys.store.provider;
+
+import java.io.IOException;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.testing.store.NoWriteLocalStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A really simple provider that stores data in the local file system, one 
value per file.
+ */
+public class LocalPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(LocalPersistentStoreProvider.class);
+
+  private final Path path;
+  private final DrillFileSystem fs;
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148015#comment-15148015
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52962203
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/CachingPersistentStoreProvider.java
 ---
@@ -0,0 +1,72 @@
+/**
+ * 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.sys.store.provider;
+
+import java.util.concurrent.ConcurrentMap;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreProvider;
+
+public class CachingPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(CachingPersistentStoreProvider.class);
+
+  private final ConcurrentMap storeCache = Maps.newConcurrentMap();
+  private final PersistentStoreProvider provider;
+
+  public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+this.provider = provider;
+  }
+
+  @SuppressWarnings("unchecked")
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
+final PersistentStore store = storeCache.get(config);
+if (store == null) {
+  final PersistentStore newStore = 
provider.getOrCreateStore(config);
+  final PersistentStore finalStore = storeCache.putIfAbsent(config, 
newStore);
+  if (finalStore == null) {
+return (PersistentStore)newStore;
+  }
+  try {
+newStore.close();
+  } catch (Exception ex) {
+throw new StoreException(ex);
+  }
+}
+
+return (PersistentStore) store;
+
+  }
+
+  @Override
+  public void start() throws Exception {
+provider.start();
+  }
+
+  @Override
+  public void close() throws Exception {
+for(final PersistentStore store : storeCache.values()){
+  store.close();
--- End diff --

AutoCloseables.close(storecache.values() + provider). Done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148013#comment-15148013
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52962000
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -146,38 +154,37 @@ public QProfiles(List runningQueries, 
List finishedQue
   @Path("/profiles.json")
   @Produces(MediaType.APPLICATION_JSON)
   public QProfiles getProfilesJSON() {
-PStore completed = null;
-PStore running = null;
-try {
-  completed = provider().getStore(QueryManager.QUERY_PROFILE);
-  running = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-} catch (IOException e) {
-  logger.debug("Failed to get profiles from persistent or ephemeral 
store.");
-  return new QProfiles(new ArrayList(), new 
ArrayList());
-}
-
-List runningQueries = Lists.newArrayList();
-
-for (Map.Entry entry : running) {
-  QueryInfo profile = entry.getValue();
-  if (principal.canManageProfileOf(profile.getUser())) {
-runningQueries.add(new ProfileInfo(entry.getKey(), 
profile.getStart(), profile.getForeman().getAddress(),
-profile.getQuery(), profile.getState().name(), 
profile.getUser()));
+try (
+final PersistentStore completed = 
getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE);
--- End diff --

Fixed.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147998#comment-15147998
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52961092
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/LocalPersistentStoreProvider.java
 ---
@@ -0,0 +1,74 @@
+/**
+ * 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.sys.store.provider;
+
+import java.io.IOException;
+
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.ExecConstants;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.testing.store.NoWriteLocalStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A really simple provider that stores data in the local file system, one 
value per file.
+ */
+public class LocalPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(LocalPersistentStoreProvider.class);
+
+  private final Path path;
+  private final DrillFileSystem fs;
--- End diff --

Shouldn't fs be closed in close()?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15148002#comment-15148002
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52961125
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/ZookeeperPersistentStoreProvider.java
 ---
@@ -0,0 +1,85 @@
+/**
+ * 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.sys.store.provider;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.coord.zk.ZKClusterCoordinator;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.store.sys.store.ZookeeperPersistentStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZookeeperPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(ZookeeperPersistentStoreProvider.class);
+
+  private static final String DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT = 
"drill.exec.sys.store.provider.zk.blobroot";
+
+  private final CuratorFramework curator;
+  private final DrillFileSystem fs;
--- End diff --

Shouldn't fs be closed in close()?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147988#comment-15147988
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960719
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/CachingPersistentStoreProvider.java
 ---
@@ -0,0 +1,72 @@
+/**
+ * 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.sys.store.provider;
+
+import java.util.concurrent.ConcurrentMap;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreProvider;
+
+public class CachingPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(CachingPersistentStoreProvider.class);
+
+  private final ConcurrentMap storeCache = Maps.newConcurrentMap();
+  private final PersistentStoreProvider provider;
+
+  public CachingPersistentStoreProvider(PersistentStoreProvider provider) {
+this.provider = provider;
+  }
+
+  @SuppressWarnings("unchecked")
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
+final PersistentStore store = storeCache.get(config);
+if (store == null) {
+  final PersistentStore newStore = 
provider.getOrCreateStore(config);
+  final PersistentStore finalStore = storeCache.putIfAbsent(config, 
newStore);
+  if (finalStore == null) {
+return (PersistentStore)newStore;
+  }
+  try {
+newStore.close();
+  } catch (Exception ex) {
+throw new StoreException(ex);
+  }
+}
+
+return (PersistentStore) store;
+
+  }
+
+  @Override
+  public void start() throws Exception {
+provider.start();
+  }
+
+  @Override
+  public void close() throws Exception {
+for(final PersistentStore store : storeCache.values()){
+  store.close();
--- End diff --

AutoCloseables.close(storeCache.values());


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147989#comment-15147989
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960723
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/provider/ZookeeperPersistentStoreProvider.java
 ---
@@ -0,0 +1,85 @@
+/**
+ * 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.sys.store.provider;
+
+import java.io.IOException;
+
+import com.google.common.annotations.VisibleForTesting;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.config.DrillConfig;
+import org.apache.drill.exec.coord.zk.ZKClusterCoordinator;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.store.dfs.DrillFileSystem;
+import org.apache.drill.exec.store.sys.PersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreRegistry;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.store.LocalPersistentStore;
+import org.apache.drill.exec.store.sys.store.ZookeeperPersistentStore;
+import org.apache.hadoop.fs.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ZookeeperPersistentStoreProvider extends 
BasePersistentStoreProvider {
+  private static final Logger logger = 
LoggerFactory.getLogger(ZookeeperPersistentStoreProvider.class);
+
+  private static final String DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT = 
"drill.exec.sys.store.provider.zk.blobroot";
+
+  private final CuratorFramework curator;
+  private final DrillFileSystem fs;
+  private final Path blobRoot;
+
+  public ZookeeperPersistentStoreProvider(final 
PersistentStoreRegistry registry) throws StoreException {
+this(registry.getConfig(), registry.getCoordinator().getCurator());
+  }
+
+  @VisibleForTesting
+  public ZookeeperPersistentStoreProvider(final DrillConfig config, final 
CuratorFramework curator) throws StoreException {
+this.curator = curator;
+
+if (config.hasPath(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT)) {
+  blobRoot = new 
Path(config.getString(DRILL_EXEC_SYS_STORE_PROVIDER_ZK_BLOBROOT));
+}else{
+  blobRoot = LocalPersistentStore.getLogDir();
+}
+
+try {
+  this.fs = LocalPersistentStore.getFileSystem(config, blobRoot);
+} catch (IOException ex) {
+  throw new StoreException("unable to get filesystem", ex);
+}
+  }
+
+  @Override
+  public  PersistentStore getOrCreateStore(final 
PersistentStoreConfig config) throws StoreException {
+switch(config.getMode()){
+case BLOB_PERSISTENT:
+  return new LocalPersistentStore<>(fs, blobRoot, config);
--- End diff --

LocalPersistentStore in ZookeeperPersistentStoreProvider?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store 

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147981#comment-15147981
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960456
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -217,11 +223,14 @@ private QueryProfile getQueryProfile(String queryId) 
throws IOException {
 }
 
 // then check blob store
-PStore profiles = 
provider().getStore(QueryManager.QUERY_PROFILE);
-QueryProfile queryProfile = profiles.get(queryId);
-if (queryProfile != null) {
-  checkOrThrowProfileViewAuthorization(queryProfile);
-  return queryProfile;
+try (final PersistentStore profiles = 
getProvider().getOrCreateStore(QueryManager.QUERY_PROFILE)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147980#comment-15147980
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52960453
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -200,9 +207,8 @@ private QueryProfile getQueryProfile(String queryId) 
throws IOException {
 }
 
 // then check remote running
-try{
-  PStore runningQueries = 
provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-  QueryInfo info = runningQueries.get(queryId);
+try (final TransientStore running = 
getCoordinator().getOrCreateTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147962#comment-15147962
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/374#issuecomment-184463952
  
Please let me know of further comments and remember to +1 if you are comfy 
with the patch. If nothing severe is seen, let's take all comments on follow-up 
PRs. This patch is long waiting I'd like to check this in. Thanks for all 
feedback.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147952#comment-15147952
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52958072
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

All included.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147949#comment-15147949
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52957626
  
--- Diff: exec/java-exec/src/main/resources/drill-module.conf ---
@@ -120,7 +120,7 @@ drill.exec: {
 affinity.factor: 1.2
   },
   sys.store.provider: {
-class: "org.apache.drill.exec.store.sys.zk.ZkPStoreProvider",
+class: 
"org.apache.drill.exec.store.sys.store.provider.ZookeeperPersistentStoreProvider",
--- End diff --

What about other *PStoreProvider? This will still impact those users.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147944#comment-15147944
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52957306
  
--- Diff: 
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java ---
@@ -0,0 +1,62 @@
+/**
+ * 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.common.collections;
+
+import java.util.Map;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class ImmutableEntry implements Map.Entry  {
+  private final K key;
+  private final V value;
+
+  public ImmutableEntry(final K key, final V value) {
+this.key = Preconditions.checkNotNull(key, "key is required");
+this.value = Preconditions.checkNotNull(value, "value is required");
+  }
+
+  @Override
+  public K getKey() {
+return key;
+  }
+
+  @Override
+  public V getValue() {
+return value;
+  }
+
+  @Override
+  public V setValue(final V value) {
+throw new UnsupportedOperationException("entry is immutable");
+  }
+
+  @Override
+  public boolean equals(final Object other) {
+if (other instanceof ImmutableEntry && other.getClass() == getClass()) 
{
+  final ImmutableEntry entry = (ImmutableEntry)other;
+  return Objects.equal(key, entry.key) && Objects.equal(value, 
entry.value);
--- End diff --

Cool. I think we should consider replacing all uses on a separate patch 
once this is deprecated.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147942#comment-15147942
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52957167
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java
 ---
@@ -206,6 +209,16 @@ public DistributedSemaphore getSemaphore(String name, 
int maximumLeases) {
 return new ZkDistributedSemaphore(curator, "/semaphore/" + name, 
maximumLeases);
   }
 
+  @Override
+  public  TransientStore getOrCreateTransientStore(final 
TransientStoreConfig config) {
+final ZkEphemeralStore store = 
(ZkEphemeralStore)factory.getOrCreateStore(config);
+try {
+  store.start();
--- End diff --

That logic now is in factory which calls start once.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147920#comment-15147920
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955476
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/LocalClusterCoordinator.java
 ---
@@ -125,6 +127,11 @@ public DistributedSemaphore getSemaphore(final String 
name, final int maximumLea
 return semaphores.get(name);
   }
 
+  @Override
+  public  TransientStore getOrCreateTransientStore(final 
TransientStoreConfig config) {
+return new MapBackedStore<>(config);
--- End diff --

I think it's quite cheap here to do so here. Added caching too.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147919#comment-15147919
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955131
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStoreProvider.java
 ---
@@ -41,10 +39,8 @@
 import org.apache.hadoop.hbase.client.HTableInterface;
 import org.apache.hadoop.hbase.util.Bytes;
 
-import com.google.common.annotations.VisibleForTesting;
-
-public class HBasePStoreProvider implements PStoreProvider {
-  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePStoreProvider.class);
+public class HBasePersistentStoreProvider extends 
BasePersistentStoreProvider {
+  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePersistentStoreProvider.class);
--- End diff --

not relevant to my changes but did my best to privatize them all ;)


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147918#comment-15147918
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955096
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZookeeperClient.java
 ---
@@ -0,0 +1,238 @@
+/**
+ * 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.coord.zk;
+
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
+import com.google.common.collect.Iterables;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.recipes.cache.ChildData;
+import org.apache.curator.framework.recipes.cache.PathChildrenCache;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.zookeeper.CreateMode;
+
+/**
+ * A namespace aware Zookeper client.
--- End diff --

seriously :+1: 


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147916#comment-15147916
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52955022
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/LocalClusterCoordinator.java
 ---
@@ -125,6 +127,11 @@ public DistributedSemaphore getSemaphore(final String 
name, final int maximumLea
 return semaphores.get(name);
   }
 
+  @Override
+  public  TransientStore getOrCreateTransientStore(final 
TransientStoreConfig config) {
+return new MapBackedStore<>(config);
--- End diff --

Looks like we are creating a new MapBackedStore for every call to this. 
There is no caching factory in the call path to this method either.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147912#comment-15147912
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954392
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStoreProvider.java
 ---
@@ -41,10 +39,8 @@
 import org.apache.hadoop.hbase.client.HTableInterface;
 import org.apache.hadoop.hbase.util.Bytes;
 
-import com.google.common.annotations.VisibleForTesting;
-
-public class HBasePStoreProvider implements PStoreProvider {
-  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePStoreProvider.class);
+public class HBasePersistentStoreProvider extends 
BasePersistentStoreProvider {
+  static final org.slf4j.Logger logger = 
org.slf4j.LoggerFactory.getLogger(HBasePersistentStoreProvider.class);
--- End diff --

_private_ for all loggers of *Provider and *Store would be nice :)


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147911#comment-15147911
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954382
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java
 ---
@@ -237,11 +237,17 @@ void unpauseExecutingFragments(final DrillbitContext 
drillbitContext) {
 }
   }
 
+  @Override
+  public void close() throws Exception {
+profileStore.close();
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147908#comment-15147908
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954150
  
--- Diff: 
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java ---
@@ -0,0 +1,62 @@
+/**
+ * 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.common.collections;
+
+import java.util.Map;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class ImmutableEntry implements Map.Entry  {
+  private final K key;
+  private final V value;
+
+  public ImmutableEntry(final K key, final V value) {
+this.key = Preconditions.checkNotNull(key, "key is required");
+this.value = Preconditions.checkNotNull(value, "value is required");
+  }
+
+  @Override
+  public K getKey() {
+return key;
+  }
+
+  @Override
+  public V getValue() {
+return value;
+  }
+
+  @Override
+  public V setValue(final V value) {
+throw new UnsupportedOperationException("entry is immutable");
+  }
+
+  @Override
+  public boolean equals(final Object other) {
+if (other instanceof ImmutableEntry && other.getClass() == getClass()) 
{
+  final ImmutableEntry entry = (ImmutableEntry)other;
+  return Objects.equal(key, entry.key) && Objects.equal(value, 
entry.value);
--- End diff --

There is no deprecated annotation, but the doc. for the guava 18 that we 
use has this note as well.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147906#comment-15147906
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52954081
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZkEphemeralStore.java
 ---
@@ -0,0 +1,145 @@
+/**
+ * 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.coord.zk;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
+import org.apache.curator.framework.CuratorFramework;
+import 
org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.zookeeper.CreateMode;
+
+public class ZkEphemeralStore extends BaseTransientStore {
+
+  @VisibleForTesting
+  protected final PathChildrenCacheListener dispatcher = new 
EventDispatcher<>(this);
+  private final ZookeeperClient client;
+
+  public ZkEphemeralStore(final TransientStoreConfig config, final 
CuratorFramework curator) {
+super(config);
+this.client = new ZookeeperClient(curator, PathUtils.join("/", 
config.getName()), CreateMode.EPHEMERAL);
+  }
+
+  public void start() throws Exception {
+getClient().getCache().getListenable().addListener(dispatcher);
+getClient().start();
+  }
+
+  protected ZookeeperClient getClient() {
+return client;
+  }
+
+  @Override
+  public V get(final String key) {
+final byte[] bytes = getClient().get(key);
+if (bytes == null) {
+  return null;
+}
+try {
+  return config.getSerializer().deserialize(bytes);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to deserialize 
value at %s", key), e);
+}
+  }
+
+  @Override
+  public V put(final String key, final V value) {
+final InstanceSerializer serializer = config.getSerializer();
+try {
+  final byte[] old = getClient().get(key);
+  final byte[] bytes = serializer.serialize(value);
+  getClient().put(key, bytes);
+  if (old == null) {
+return null;
+  }
+  return serializer.deserialize(old);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to 
de/serialize value of type %s", value.getClass()), e);
+}
+  }
+
+  @Override
+  public V putIfAbsent(final String key, final V value) {
+final V old = get(key);
+if (old == null) {
+  try {
+final byte[] bytes = config.getSerializer().serialize(value);
+getClient().put(key, bytes);
+  } catch (final IOException e) {
+throw new DrillRuntimeException(String.format("unable to serialize 
value of type %s", value.getClass()), e);
+  }
+}
+return old;
+  }
+
+  @Override
+  public V remove(final String key) {
+final V existing = get(key);
+if (existing != null) {
+  getClient().delete(key);
+}
+return existing;
+  }
+
+  @Override
+  public Iterator> entries() {
+return 

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147890#comment-15147890
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953099
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStore.java
 ---
@@ -0,0 +1,77 @@
+/**
+ * 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.sys;
+
+import java.util.Iterator;
+import java.util.Map;
+
+/**
+ * An abstraction used to store and retrieve instances of given value type.
+ *
+ * @param   value type
+ */
+public interface PersistentStore extends AutoCloseable {
--- End diff --

Any reason you chose to remove extending _java.lang.Iterable_? Most usages 
now look like Lists.newArrayList(store.getAll())


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147889#comment-15147889
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953084
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -267,9 +277,8 @@ public String cancelQuery(@PathParam("queryid") String 
queryId) throws IOExcepti
 }
 
 // then check remote running
-try{
-  PStore runningQueries = 
provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-  QueryInfo info = runningQueries.get(queryId);
+try (final TransientStore running = 
getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147886#comment-15147886
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953078
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -146,38 +155,37 @@ public QProfiles(List runningQueries, 
List finishedQue
   @Path("/profiles.json")
   @Produces(MediaType.APPLICATION_JSON)
   public QProfiles getProfilesJSON() {
-PStore completed = null;
-PStore running = null;
-try {
-  completed = provider().getStore(QueryManager.QUERY_PROFILE);
-  running = provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-} catch (IOException e) {
-  logger.debug("Failed to get profiles from persistent or ephemeral 
store.");
-  return new QProfiles(new ArrayList(), new 
ArrayList());
-}
-
-List runningQueries = Lists.newArrayList();
-
-for (Map.Entry entry : running) {
-  QueryInfo profile = entry.getValue();
-  if (principal.canManageProfileOf(profile.getUser())) {
-runningQueries.add(new ProfileInfo(entry.getKey(), 
profile.getStart(), profile.getForeman().getAddress(),
-profile.getQuery(), profile.getState().name(), 
profile.getUser()));
+try (
+final PersistentStore completed = 
getProvider().getStore(QueryManager.QUERY_PROFILE);
+final TransientStore running = 
getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO);
--- End diff --

This isn't creating a new store, right?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147891#comment-15147891
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953108
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/PersistentStoreProvider.java
 ---
@@ -17,17 +17,26 @@
  */
 package org.apache.drill.exec.store.sys;
 
-import java.util.Map;
-
+import org.apache.drill.exec.exception.StoreException;
 
 /**
- * Interface for reading and writing values to a persistent storage 
provider.  Iterators are guaranteed to be returned in key order.
- * @param 
+ * A factory used to create {@link PersistentStore store} instances.
+ *
  */
-public interface PStore extends Iterable> {
-  public V get(String key);
-  public void put(String key, V value);
-  public boolean putIfAbsent(String key, V value);
-  public void delete(String key);
-  public void close();
+public interface PersistentStoreProvider extends AutoCloseable {
+  /**
+   * Gets or creates a {@link PersistentStore persistent store} for the 
given configuration.
+   *
+   * Note that implementors have liberty to cache previous {@link 
PersistentStore store} instances.
+   *
+   * @param config  store configuration
+   * @param   store value type
+   */
+   PersistentStore getStore(PersistentStoreConfig config) throws 
StoreException;
--- End diff --

getOrCreateStore(...)?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147888#comment-15147888
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953080
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/profile/ProfileResources.java
 ---
@@ -200,9 +208,8 @@ private QueryProfile getQueryProfile(String queryId) 
throws IOException {
 }
 
 // then check remote running
-try{
-  PStore runningQueries = 
provider().getStore(QueryManager.RUNNING_QUERY_INFO);
-  QueryInfo info = runningQueries.get(queryId);
+try (final TransientStore running = 
getCoordinator().newTransientStore(QueryManager.RUNNING_QUERY_INFO)) {
--- End diff --

same here


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Updated] (DRILL-4386) Unify and move serialization logic to common module or package

[Hanifi Gunes (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-4386?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Hanifi Gunes updated DRILL-4386:

Labels: easy starter  (was: starter)

> Unify and move serialization logic to common module or package
> --
>
> Key: DRILL-4386
> URL: https://issues.apache.org/jira/browse/DRILL-4386
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Codegen
>Affects Versions: 1.5.0
>Reporter: Hanifi Gunes
>Priority: Minor
>  Labels: easy, starter
>
> In many places around Drill we rely on custom SerDes. However, there seems 
> some redundancy. For instance, Transient/PersistentStore(introduced by 
> DRILL-4275) relies on InstanceSerializer whereas Controller uses CustomSerDe 
> interface. Effectively these two are the same. We need to unify use cases, 
> possibly moving SerDe logic to common module or some level up in the exec 
> module. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147885#comment-15147885
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953064
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZkEphemeralStore.java
 ---
@@ -0,0 +1,145 @@
+/**
+ * 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.coord.zk;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
+import com.google.common.collect.Iterators;
+import com.google.common.collect.Lists;
+import org.apache.curator.framework.CuratorFramework;
+import 
org.apache.curator.framework.recipes.cache.PathChildrenCacheListener;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.zookeeper.CreateMode;
+
+public class ZkEphemeralStore extends BaseTransientStore {
+
+  @VisibleForTesting
+  protected final PathChildrenCacheListener dispatcher = new 
EventDispatcher<>(this);
+  private final ZookeeperClient client;
+
+  public ZkEphemeralStore(final TransientStoreConfig config, final 
CuratorFramework curator) {
+super(config);
+this.client = new ZookeeperClient(curator, PathUtils.join("/", 
config.getName()), CreateMode.EPHEMERAL);
+  }
+
+  public void start() throws Exception {
+getClient().getCache().getListenable().addListener(dispatcher);
+getClient().start();
+  }
+
+  protected ZookeeperClient getClient() {
+return client;
+  }
+
+  @Override
+  public V get(final String key) {
+final byte[] bytes = getClient().get(key);
+if (bytes == null) {
+  return null;
+}
+try {
+  return config.getSerializer().deserialize(bytes);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to deserialize 
value at %s", key), e);
+}
+  }
+
+  @Override
+  public V put(final String key, final V value) {
+final InstanceSerializer serializer = config.getSerializer();
+try {
+  final byte[] old = getClient().get(key);
+  final byte[] bytes = serializer.serialize(value);
+  getClient().put(key, bytes);
+  if (old == null) {
+return null;
+  }
+  return serializer.deserialize(old);
+} catch (final IOException e) {
+  throw new DrillRuntimeException(String.format("unable to 
de/serialize value of type %s", value.getClass()), e);
+}
+  }
+
+  @Override
+  public V putIfAbsent(final String key, final V value) {
+final V old = get(key);
+if (old == null) {
+  try {
+final byte[] bytes = config.getSerializer().serialize(value);
+getClient().put(key, bytes);
+  } catch (final IOException e) {
+throw new DrillRuntimeException(String.format("unable to serialize 
value of type %s", value.getClass()), e);
+  }
+}
+return old;
+  }
+
+  @Override
+  public V remove(final String key) {
+final V existing = get(key);
+if (existing != null) {
+  getClient().delete(key);
+}
+return existing;
+  }
+
+  @Override
+  public Iterator> entries() {
+return 

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147883#comment-15147883
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953044
  
--- Diff: 
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/config/HBasePersistentStore.java
 ---
@@ -148,13 +154,13 @@ private void delete(byte[] row) {
 private Result current = null;
 private Result last = null;
 private boolean done = false;
-private int rowsRead = 0;
 
-Iter() {
+Iter(int take) {
   try {
 Scan scan = new Scan(tableNameStartKey, tableNameStopKey);
 scan.addColumn(FAMILY, QUALIFIER);
-scan.setCaching(config.getMaxIteratorSize() > 100 ? 100 : 
config.getMaxIteratorSize());
+scan.setCaching(Math.min(take, 100));
--- End diff --

Is _config.getMaxIteratorSize()_ not required?


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147882#comment-15147882
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953040
  
--- Diff: 
common/src/main/java/org/apache/drill/common/collections/ImmutableEntry.java ---
@@ -0,0 +1,62 @@
+/**
+ * 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.common.collections;
+
+import java.util.Map;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+
+public class ImmutableEntry implements Map.Entry  {
+  private final K key;
+  private final V value;
+
+  public ImmutableEntry(final K key, final V value) {
+this.key = Preconditions.checkNotNull(key, "key is required");
+this.value = Preconditions.checkNotNull(value, "value is required");
+  }
+
+  @Override
+  public K getKey() {
+return key;
+  }
+
+  @Override
+  public V getValue() {
+return value;
+  }
+
+  @Override
+  public V setValue(final V value) {
+throw new UnsupportedOperationException("entry is immutable");
+  }
+
+  @Override
+  public boolean equals(final Object other) {
+if (other instanceof ImmutableEntry && other.getClass() == getClass()) 
{
+  final ImmutableEntry entry = (ImmutableEntry)other;
+  return Objects.equal(key, entry.key) && Objects.equal(value, 
entry.value);
--- End diff --

google.common.base.Objects#equal and #hashCode [should be treated as 
deprecated](https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Objects.java#L55).


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Updated] (DRILL-4386) Unify and move serialization logic to common module or package

[Hanifi Gunes (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-4386?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Hanifi Gunes updated DRILL-4386:

Labels: starter  (was: #starter)

> Unify and move serialization logic to common module or package
> --
>
> Key: DRILL-4386
> URL: https://issues.apache.org/jira/browse/DRILL-4386
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Codegen
>Affects Versions: 1.5.0
>Reporter: Hanifi Gunes
>Priority: Minor
>  Labels: easy, starter
>
> In many places around Drill we rely on custom SerDes. However, there seems 
> some redundancy. For instance, Transient/PersistentStore(introduced by 
> DRILL-4275) relies on InstanceSerializer whereas Controller uses CustomSerDe 
> interface. Effectively these two are the same. We need to unify use cases, 
> possibly moving SerDe logic to common module or some level up in the exec 
> module. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147884#comment-15147884
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user sudheeshkatkam commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52953057
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
 ---
@@ -0,0 +1,22 @@
+/**
+ * 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.coord.store;
+
+public interface TransientStoreListener {
--- End diff --

javadoc


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Created] (DRILL-4386) Unify and move serialization logic to common module or package

[Hanifi Gunes (JIRA)]

Hanifi Gunes created DRILL-4386:
---

 Summary: Unify and move serialization logic to common module or 
package
 Key: DRILL-4386
 URL: https://issues.apache.org/jira/browse/DRILL-4386
 Project: Apache Drill
  Issue Type: Improvement
  Components: Execution - Codegen
Affects Versions: 1.5.0
Reporter: Hanifi Gunes
Priority: Minor


In many places around Drill we rely on custom SerDes. However, there seems some 
redundancy. For instance, Transient/PersistentStore(introduced by DRILL-4275) 
relies on InstanceSerializer whereas Controller uses CustomSerDe interface. 
Effectively these two are the same. We need to unify use cases, possibly moving 
SerDe logic to common module or some level up in the exec module. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Updated] (DRILL-4386) Unify and move serialization logic to common module or package

[Hanifi Gunes (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-4386?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Hanifi Gunes updated DRILL-4386:

Labels: #starter  (was: )

> Unify and move serialization logic to common module or package
> --
>
> Key: DRILL-4386
> URL: https://issues.apache.org/jira/browse/DRILL-4386
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Codegen
>Affects Versions: 1.5.0
>Reporter: Hanifi Gunes
>Priority: Minor
>  Labels: #starter
>
> In many places around Drill we rely on custom SerDes. However, there seems 
> some redundancy. For instance, Transient/PersistentStore(introduced by 
> DRILL-4275) relies on InstanceSerializer whereas Controller uses CustomSerDe 
> interface. Effectively these two are the same. We need to unify use cases, 
> possibly moving SerDe logic to common module or some level up in the exec 
> module. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147877#comment-15147877
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on the pull request:

https://github.com/apache/drill/pull/374#issuecomment-184436146
  
I have retracted unrelated mongo changes. Thanks for the feedback.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147866#comment-15147866
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52951541
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/store/TransientStoreListener.java
 ---
@@ -0,0 +1,22 @@
+/**
+ * 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.coord.store;
+
+public interface TransientStoreListener {
+  void onChange(TransientStoreEvent event);
--- End diff --

I typically prefer this way as it allows extending capabilities(esp event 
types) via polymorphism without requiring to alter the interface. I am fine 
with either way though.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147860#comment-15147860
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52951273
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/PathUtils.java ---
@@ -0,0 +1,59 @@
+/**
+ * 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.coord.zk;
+
+import com.google.common.base.Preconditions;
+import org.apache.parquet.Strings;
+
+public final class PathUtils {
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147857#comment-15147857
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950999
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/ClusterCoordinator.java
 ---
@@ -60,16 +62,23 @@
   public abstract DistributedSemaphore getSemaphore(String name, int 
maximumLeases);
 
   /**
+   * Returns a new {@link TransientStore store} instance with the given 
{@link TransientStoreConfig configuration}.
+   * @param config  store configuration
+   * @param   value type for this store
+   */
+  public abstract  TransientStore 
newTransientStore(TransientStoreConfig config);
+
+  /**
* Actions to take when there are a set of new de-active drillbits.
* @param unregisteredBits
*/
-  public void drillbitUnregistered(Set unregisteredBits) 
{
+  protected void drillbitUnregistered(Set 
unregisteredBits) {
--- End diff --

Wrong use of this method outside of subclasses could easily mess the system 
up. To my understanding, this method should only be visible to implementor who 
is the ultimate authority to fire listeners on topology changes, so protected 
seems to make more sense here.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147855#comment-15147855
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950576
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/MapBackedStore.java
 ---
@@ -0,0 +1,86 @@
+/**
+ * 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.coord.local;
+
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+
+import com.google.common.collect.Maps;
+import org.apache.drill.exec.coord.store.BaseTransientStore;
+import org.apache.drill.exec.coord.store.TransientStoreConfig;
+import org.apache.drill.exec.coord.store.TransientStoreEvent;
+import org.apache.drill.exec.coord.store.TransientStoreEventType;
+
+public class MapBackedStore extends BaseTransientStore {
+  private final HashMap delegate = Maps.newHashMap();
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147854#comment-15147854
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950554
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/serialization/JacksonSerializer.java
 ---
@@ -0,0 +1,59 @@
+/**
+ * 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.serialization;
+
+import java.io.IOException;
+import java.util.Objects;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.ObjectWriter;
+
+public class JacksonSerializer implements InstanceSerializer {
+  private final ObjectReader reader;
+  private final ObjectWriter writer;
+
+  public JacksonSerializer(final ObjectMapper mapper, final Class 
klazz) {
+this.reader = mapper.reader(klazz);
+this.writer = mapper.writer();
+  }
+
+  @Override
+  public T deserialize(final byte[] raw) throws IOException {
+return reader.readValue(raw);
+  }
+
+  @Override
+  public byte[] serialize(final T instance) throws IOException {
+return writer.writeValueAsBytes(instance);
+  }
+
+  @Override
+  public boolean equals(Object obj) {
+if (obj instanceof JacksonSerializer && 
obj.getClass().equals(getClass())) {
+  final JacksonSerializer other = (JacksonSerializer)obj;
+  return Objects.equals(reader, other.reader) && 
Objects.equals(writer, other.writer);
+}
+return false;
+  }
+
+  @Override
+  public int hashCode() {
+return super.hashCode();
--- End diff --

nice catch. thanks. done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147853#comment-15147853
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950537
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/store/sys/store/ZookeeperPersistentStore.java
 ---
@@ -0,0 +1,136 @@
+/**
+ * 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.sys.store;
+
+import java.io.IOException;
+import java.util.Iterator;
+import java.util.Map;
+
+import javax.annotation.Nullable;
+
+import com.google.common.base.Function;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Iterators;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.drill.common.collections.ImmutableEntry;
+import org.apache.drill.common.exceptions.DrillRuntimeException;
+import org.apache.drill.exec.coord.zk.PathUtils;
+import org.apache.drill.exec.coord.zk.ZookeeperClient;
+import org.apache.drill.exec.exception.StoreException;
+import org.apache.drill.exec.serialization.InstanceSerializer;
+import org.apache.drill.exec.store.sys.BasePersistentStore;
+import org.apache.drill.exec.store.sys.PersistentStoreConfig;
+import org.apache.drill.exec.store.sys.PersistentStoreMode;
+import org.apache.zookeeper.CreateMode;
+
+/**
+ * This is the abstract class that is shared by ZkPStore (Persistent 
store) and ZkEStore (Ephemeral Store)
--- End diff --

done


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4275) Refactor e/pstore interfaces and their factories to provide a unified mechanism to access stores

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4275?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147852#comment-15147852
 ] 

ASF GitHub Bot commented on DRILL-4275:
---

Github user hnfgns commented on a diff in the pull request:

https://github.com/apache/drill/pull/374#discussion_r52950533
  
--- Diff: 
exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/ZKClusterCoordinator.java
 ---
@@ -206,6 +205,16 @@ public DistributedSemaphore getSemaphore(String name, 
int maximumLeases) {
 return new ZkDistributedSemaphore(curator, "/semaphore/" + name, 
maximumLeases);
   }
 
+  @Override
+  public  TransientStore newTransientStore(final 
TransientStoreConfig config) {
+final ZkEphemeralStore store = new ZkEphemeralStore<>(config, 
curator);
--- End diff --

done.


> Refactor e/pstore interfaces and their factories to provide a unified 
> mechanism to access stores
> 
>
> Key: DRILL-4275
> URL: https://issues.apache.org/jira/browse/DRILL-4275
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Execution - Flow
>Reporter: Hanifi Gunes
>Assignee: Deneche A. Hakim
>
> We rely on E/PStore interfaces to persist data. Even though E/PStore stands 
> for Ephemeral and Persistent stores respectively, the current design for 
> EStore does not extend the interface/functionality of PStore at all, which 
> hints abstraction for EStore is redundant. This issue proposes a new unified 
> Store interface replacing the old E/PStore that exposes an additional method 
> that report persistence level as follows:
> {code:title=Store interface}
> interface Store {
>   StoreMode getMode();
>   V get(String key);
>   ...
> }
> enum StoreMode {
>   EPHEMERAL,
>   PERSISTENT,
>   ...
> }
> {code}
> The new design brings in less redundancy, more centralized code, ease to 
> reason and maintain.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-3688) Drill should honor "skip.header.line.count" attribute of Hive table

[Arina Ielchiieva (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-3688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147813#comment-15147813
 ] 

Arina Ielchiieva commented on DRILL-3688:
-

Hive also has similar property "skip.footer.line.count", do we need to support 
it, too?

> Drill should honor "skip.header.line.count" attribute of Hive table
> ---
>
> Key: DRILL-3688
> URL: https://issues.apache.org/jira/browse/DRILL-3688
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Storage - Hive
>Affects Versions: 1.1.0
> Environment: 1.1
>Reporter: Hao Zhu
>Assignee: Arina Ielchiieva
> Fix For: Future
>
>
> Currently Drill does not honor the "skip.header.line.count" attribute of Hive 
> table.
> It may cause some other format conversion issue.
> Reproduce:
> 1. Create a Hive table
> {code}
> create table h1db.testheader(col0 string)
> ROW FORMAT DELIMITED FIELDS TERMINATED BY '|'
> STORED AS TEXTFILE
> tblproperties("skip.header.line.count"="1");
> {code}
> 2. Prepare a sample data:
> {code}
> # cat test.data
> col0
> 2015-01-01
> {code}
> 3. Load sample data into Hive
> {code}
> LOAD DATA LOCAL INPATH '/xxx/test.data' OVERWRITE INTO TABLE h1db.testheader;
> {code}
> 4. Hive
> {code}
> hive> select * from h1db.testheader ;
> OK
> 2015-01-01
> Time taken: 0.254 seconds, Fetched: 1 row(s)
> {code}
> 5. Drill
> {code}
> >  select * from hive.h1db.testheader ;
> +-+
> |col0 |
> +-+
> | col0|
> | 2015-01-01  |
> +-+
> 2 rows selected (0.257 seconds)
> > select cast(col0 as date) from hive.h1db.testheader ;
> Error: SYSTEM ERROR: IllegalFieldValueException: Value 0 for monthOfYear must 
> be in the range [1,12]
> Fragment 0:0
> [Error Id: 34353702-ca27-440b-a4f4-0c9f79fc8ccd on h1.poc.com:31010]
>   (org.joda.time.IllegalFieldValueException) Value 0 for monthOfYear must be 
> in the range [1,12]
> org.joda.time.field.FieldUtils.verifyValueBounds():236
> org.joda.time.chrono.BasicChronology.getDateMidnightMillis():613
> org.joda.time.chrono.BasicChronology.getDateTimeMillis():159
> org.joda.time.chrono.AssembledChronology.getDateTimeMillis():120
> org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.memGetDate():261
> org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.getDate():218
> org.apache.drill.exec.test.generated.ProjectorGen0.doEval():67
> org.apache.drill.exec.test.generated.ProjectorGen0.projectRecords():62
> 
> org.apache.drill.exec.physical.impl.project.ProjectRecordBatch.doWork():172
> org.apache.drill.exec.record.AbstractSingleRecordBatch.innerNext():93
> 
> org.apache.drill.exec.physical.impl.project.ProjectRecordBatch.innerNext():129
> org.apache.drill.exec.record.AbstractRecordBatch.next():147
> org.apache.drill.exec.physical.impl.BaseRootExec.next():83
> 
> org.apache.drill.exec.physical.impl.ScreenCreator$ScreenRoot.innerNext():79
> org.apache.drill.exec.physical.impl.BaseRootExec.next():73
> org.apache.drill.exec.work.fragment.FragmentExecutor$1.run():261
> org.apache.drill.exec.work.fragment.FragmentExecutor$1.run():255
> java.security.AccessController.doPrivileged():-2
> javax.security.auth.Subject.doAs():422
> org.apache.hadoop.security.UserGroupInformation.doAs():1566
> org.apache.drill.exec.work.fragment.FragmentExecutor.run():255
> org.apache.drill.common.SelfCleaningRunnable.run():38
> java.util.concurrent.ThreadPoolExecutor.runWorker():1142
> java.util.concurrent.ThreadPoolExecutor$Worker.run():617
> java.lang.Thread.run():745 (state=,code=0)
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-3688) Drill should honor "skip.header.line.count" attribute of Hive table

[Arina Ielchiieva (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-3688?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147809#comment-15147809
 ] 

Arina Ielchiieva commented on DRILL-3688:
-

Since Hive table properties take String, such property can be set as with 
non-numeric value.
Ex:
{code}
create table h1db.testheader(col0 string)
ROW FORMAT DELIMITED FIELDS TERMINATED BY '|'
STORED AS TEXTFILE
tblproperties("skip.header.line.count"="");
{code}
Hive allows to create such table, and even load data into it. But when you 
query such table, Hive throws NumberFormatException: For input string: ""
Since Drill doesn't select from Hive directly, we can handle such case and 
ignore incorrect property, i.e. replace incorrect property value to 0.
Which behavior is better to choose: fail as Hive or let user query the data?


> Drill should honor "skip.header.line.count" attribute of Hive table
> ---
>
> Key: DRILL-3688
> URL: https://issues.apache.org/jira/browse/DRILL-3688
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Storage - Hive
>Affects Versions: 1.1.0
> Environment: 1.1
>Reporter: Hao Zhu
>Assignee: Arina Ielchiieva
> Fix For: Future
>
>
> Currently Drill does not honor the "skip.header.line.count" attribute of Hive 
> table.
> It may cause some other format conversion issue.
> Reproduce:
> 1. Create a Hive table
> {code}
> create table h1db.testheader(col0 string)
> ROW FORMAT DELIMITED FIELDS TERMINATED BY '|'
> STORED AS TEXTFILE
> tblproperties("skip.header.line.count"="1");
> {code}
> 2. Prepare a sample data:
> {code}
> # cat test.data
> col0
> 2015-01-01
> {code}
> 3. Load sample data into Hive
> {code}
> LOAD DATA LOCAL INPATH '/xxx/test.data' OVERWRITE INTO TABLE h1db.testheader;
> {code}
> 4. Hive
> {code}
> hive> select * from h1db.testheader ;
> OK
> 2015-01-01
> Time taken: 0.254 seconds, Fetched: 1 row(s)
> {code}
> 5. Drill
> {code}
> >  select * from hive.h1db.testheader ;
> +-+
> |col0 |
> +-+
> | col0|
> | 2015-01-01  |
> +-+
> 2 rows selected (0.257 seconds)
> > select cast(col0 as date) from hive.h1db.testheader ;
> Error: SYSTEM ERROR: IllegalFieldValueException: Value 0 for monthOfYear must 
> be in the range [1,12]
> Fragment 0:0
> [Error Id: 34353702-ca27-440b-a4f4-0c9f79fc8ccd on h1.poc.com:31010]
>   (org.joda.time.IllegalFieldValueException) Value 0 for monthOfYear must be 
> in the range [1,12]
> org.joda.time.field.FieldUtils.verifyValueBounds():236
> org.joda.time.chrono.BasicChronology.getDateMidnightMillis():613
> org.joda.time.chrono.BasicChronology.getDateTimeMillis():159
> org.joda.time.chrono.AssembledChronology.getDateTimeMillis():120
> org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.memGetDate():261
> org.apache.drill.exec.expr.fn.impl.StringFunctionHelpers.getDate():218
> org.apache.drill.exec.test.generated.ProjectorGen0.doEval():67
> org.apache.drill.exec.test.generated.ProjectorGen0.projectRecords():62
> 
> org.apache.drill.exec.physical.impl.project.ProjectRecordBatch.doWork():172
> org.apache.drill.exec.record.AbstractSingleRecordBatch.innerNext():93
> 
> org.apache.drill.exec.physical.impl.project.ProjectRecordBatch.innerNext():129
> org.apache.drill.exec.record.AbstractRecordBatch.next():147
> org.apache.drill.exec.physical.impl.BaseRootExec.next():83
> 
> org.apache.drill.exec.physical.impl.ScreenCreator$ScreenRoot.innerNext():79
> org.apache.drill.exec.physical.impl.BaseRootExec.next():73
> org.apache.drill.exec.work.fragment.FragmentExecutor$1.run():261
> org.apache.drill.exec.work.fragment.FragmentExecutor$1.run():255
> java.security.AccessController.doPrivileged():-2
> javax.security.auth.Subject.doAs():422
> org.apache.hadoop.security.UserGroupInformation.doAs():1566
> org.apache.drill.exec.work.fragment.FragmentExecutor.run():255
> org.apache.drill.common.SelfCleaningRunnable.run():38
> java.util.concurrent.ThreadPoolExecutor.runWorker():1142
> java.util.concurrent.ThreadPoolExecutor$Worker.run():617
> java.lang.Thread.run():745 (state=,code=0)
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (DRILL-4143) REFRESH TABLE METADATA - Permission Issues with metadata files

[John Omernik (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4143?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147712#comment-15147712
 ] 

John Omernik commented on DRILL-4143:
-

After checking on this issue on Drill 1.4, and running things and having it 
worked, I thought all was well, until I loaded more data in the table, and then 
it broke again.  Here are some of the steps I took:

 I tried to refresh table metadata on the table, using the dataadm user 
(basically the user who owns the data). Note all directories and files are 
owned by dataadm:dataadm and the permissions are 770.  

On a fresh table, I ran

REFRESH TABLE METADATA mytable;

And it worked. 

Then I loaded another day worth of data, 2016-02-15, and I try to refresh the 
table only to get a permisisons error on 2015-11-12 (this day was not 
changed/affected by the load)

Error:

"false| Error: 2126.29602.2546226 
/data/prod/mytable/2015-011-12/.drill.parquet_metadata (Permission denied)12:44

This is the SAME shell where I ran it before, and I loaded more data (note the 
directory in question was already loaded, that was no touched).

Then I use the find command to remove all the .drill.parquet_metadata files. 
and run the REFRESH TABLE METADATA command again:

This time the command works. Great. 

If I run it again, right after: It runs successfully again. 

12:35  Ran it a third time, and it worked. 
12:37 Ran it a fourth time: and it worked. (Note all the parquet_metadata files 
are owned by my drillbituser: drillbitgroup (in this case, mapr:mapr) despite 
the metadata operation being done by the data owner in the drill shell)
12:39 Another process *running as dataadm* loaded a new day of data 
(2016-02-12)  No other data was altered here. 
12:40 Ran REFRESH TABLE METADATA a fifth time: Got the error. Maybe it has to 
do with adding data? Error on 2015-11-12 again
12:41 A new Process loaded more data.  (2016-02-11, and 2016-02-10 loaded) 
Process completes successfully, disabled at this time. for troubleshooting (not 
more data being loaded)
12:42 Attempt REFRESH TABLE METADATA again, same error on 2015-11-12
12:43 Removed all .drill.parquet_metadata files using find command
12:44 Ran REFRESH TABLE METADATA - This time ran with success.  Will now run 
and check without data loading. May have to do with data loading...
12:52 Ran REFRESH: Success
12:58 Ran REFRESH: Success
1:00 Forced Reload of 2016-02-15.  Basically making it so the folder 
"2016-02-15" did not have a .drill.parquet_metadata file (while the other days 
did)
1:01 Ran REFRESH : Error: 2126.27460.2555888 
/data/prod/mytable/2015-11-12/.drill.parquet_metadata (Permission denied) (Same 
file, not sure why it picks on this file, nothing is changed there) (Even 
validated, no files modified since 12:58 when the parquet_metadata file was 
modified, all parquet files still have the same modified times of when they 
were loaded, Feb 9th)



So thoughts:

1. When running REFRESH TABLE METADATA, it checks to see if all the files in 
the subdirectories exist, if they don't it starts to "do things"
2. The date 2015-11-12 probably keeps coming out is because it's first in 
.drill.parquet_metadata located in /mytable (not in the individual directories)
3. After the REFRESH failed, I checked some files. 
2015-11-12/.drill.parquet_metadata was a 0 size files. (Like it was attempted 
to be rewritten and failed) Looking in 2016-11-13, the .drill.parquet_metadata 
file has data in it. 
4. To test #3, I rm .drill.parquet_metadata from 2015-11-12, and run the 
refresh command again. Interesting... when I do that, I get permission denied 
on the 2015-11-12 directory again, this time, instead of the file owned by the 
driillbit user (and having the drillbit user group, in this case mapr)  I have 
a file of 0 bytes, with "dataadm:datareaders"  as the owner. That's 
interesting... shouldn't it be mapr:mapr (the drillbit user?)

So this seems to be the crux of the issue... what should happen here? all 
metadata operations be checked to see if the user issuing it has permissions, 
and then writes happening as the drillbit user?  

> REFRESH TABLE METADATA - Permission Issues with metadata files
> --
>
> Key: DRILL-4143
> URL: https://issues.apache.org/jira/browse/DRILL-4143
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Storage - Parquet
>Affects Versions: 1.3.0, 1.4.0
>Reporter: John Omernik
>  Labels: Metadata, Parquet, Permissions
> Fix For: Future
>
>
> Summary of Refresh Metadata Issues confirmed by two different users on Drill 
> User Mailing list. (Title: REFRESH TABLE METADATA - Access Denied)
> This issue pertains to table METADATA and revolves around user 
> authentication. 
> Basically, when the drill bits are running as one user, and the data is owned 
> by 

[jira] [Commented] (DRILL-4201) DrillPushFilterPastProject should allow partial filter pushdown.

[Victoria Markman (JIRA)]

[ 
https://issues.apache.org/jira/browse/DRILL-4201?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15147610#comment-15147610
 ] 

Victoria Markman commented on DRILL-4201:
-

I verified that partial filter is getting pushed down, however it is not going 
to happen always. It depends on the costing and heuristic there is a bit tricky.

In the case below, filter is not going to be pushed pass project, because file 
vicky.json contains only 2 rows:
{code}
0: jdbc:drill:schema=dfs> explain plan for select
. . . . . . . . . . . . > *
. . . . . . . . . . . . > from
. . . . . . . . . . . . > hive.lineitem_text_hive l
. . . . . . . . . . . . > inner join
. . . . . . . . . . . . > ( select
. . . . . . . . . . . . > flatten(test)   as test,
. . . . . . . . . . . . > o_orderkey  as orderkey
. . . . . . . . . . . . > from
. . . . . . . . . . . . > 
dfs.`/drill/testdata/Tpch0.01/json/orders/vicky.json`) as o
. . . . . . . . . . . . > on ( l.l_orderkey = o.orderkey )
. . . . . . . . . . . . > where test = 1 and o.orderkey = 22;
+--+--+
| text | json |
+--+--+
| 00-00Screen
00-01  Project(l_orderkey=[$0], l_partkey=[$1], l_suppkey=[$2], 
l_linenumber=[$3], l_quantity=[$4], l_extendedprice=[$5], l_discount=[$6], 
l_tax=[$7], l_returnflag=[$8], l_linestatus=[$9], l_shipdate=[$10], 
l_commitdate=[$11], l_receiptdate=[$12], l_shipinstruct=[$13], 
l_shipmode=[$14], l_comment=[$15], test=[$16], orderkey=[$17])
00-02Project(l_orderkey=[$0], l_partkey=[$1], l_suppkey=[$2], 
l_linenumber=[$3], l_quantity=[$4], l_extendedprice=[$5], l_discount=[$6], 
l_tax=[$7], l_returnflag=[$8], l_linestatus=[$9], l_shipdate=[$10], 
l_commitdate=[$11], l_receiptdate=[$12], l_shipinstruct=[$13], 
l_shipmode=[$14], l_comment=[$15], test=[$16], orderkey=[$17])
00-03  HashJoin(condition=[=($0, $17)], joinType=[inner])
00-05Scan(groupscan=[HiveScan [table=Table(dbName:default, 
tableName:lineitem_text_hive), columns=[`*`], numPartitions=0, partitions= 
null, 
inputDirectories=[maprfs:/drill/testdata/partition_pruning/hive/text/lineitem]]])
00-04SelectionVectorRemover
00-06  Filter(condition=[AND(=($0, 1), =($1, 22))])
00-07Flatten(flattenField=[$0])
00-08  Project(test=[$1], orderkey=[$0])
00-09Scan(groupscan=[EasyGroupScan 
[selectionRoot=maprfs:/drill/testdata/Tpch0.01/json/orders/vicky.json, 
numFiles=1, columns=[`test`, `o_orderkey`], 
files=[maprfs:///drill/testdata/Tpch0.01/json/orders/vicky.json]]])
{code}

It's not going to be pushed pass project even if I add 40 columns to be 
projected (json file with 2 rows):
{code}
0: jdbc:drill:schema=dfs> explain plan for select
. . . . . . . . . . . . > *
. . . . . . . . . . . . > from
. . . . . . . . . . . . > hive.lineitem_text_hive l
. . . . . . . . . . . . > inner join
. . . . . . . . . . . . > ( select
. . . . . . . . . . . . > flatten(test)   as test,
. . . . . . . . . . . . > o_orderkey  as orderkey,
. . . . . . . . . . . . > o_orderkey + 1  as o1,
. . . . . . . . . . . . > o_orderkey + 2  as o2,
. . . . . . . . . . . . > o_orderkey + 3  as o3,
. . . . . . . . . . . . > o_orderkey + 4  as o4,
. . . . . . . . . . . . > o_orderkey + 5  as o5,
. . . . . . . . . . . . > o_orderkey + 6  as o6,
. . . . . . . . . . . . > o_orderkey + 7  as o7,
. . . . . . . . . . . . > o_orderkey + 8  as o8,
. . . . . . . . . . . . > o_orderkey + 9  as o9,
. . . . . . . . . . . . > o_orderkey + 10 as o10,
. . . . . . . . . . . . > o_orderkey + 11 as o11,
. . . . . . . . . . . . > o_orderkey + 12 as o12,
. . . . . . . . . . . . > o_orderkey + 13 as o13,
. . . . . . . . . . . . > o_orderkey + 14 as o14,
. . . . . . . . . . . . > o_orderkey + 15 as o15,
. . . . . . . . . . . . > o_orderkey + 16 as o16,
. . . . . . . . . . . . > o_orderkey + 17 as o17,
. . . . . . . . . . . . > o_orderkey + 18 as o18,
. . . . . . . . . . . . > o_orderkey + 19 as o19,
. . . . . . . . . . . . > o_orderkey + 20 as o20,
. . . . . . . . . . . . > o_orderkey + 21 as o21,
. . . . . . . . . . . . > o_orderkey + 22 as o22,
. . . . . . . . . . . . > o_orderkey + 23 as o23,
. . . . . . . . . . . . > o_orderkey + 24 as 

[jira] [Closed] (DRILL-4169) Upgrade Hive Storage Plugin to work with latest stable Hive (v1.2.1)

[Abhishek Girish (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-4169?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Abhishek Girish closed DRILL-4169.
--

Verified.

> Upgrade Hive Storage Plugin to work with latest stable Hive (v1.2.1)
> 
>
> Key: DRILL-4169
> URL: https://issues.apache.org/jira/browse/DRILL-4169
> Project: Apache Drill
>  Issue Type: Improvement
>  Components: Storage - Hive
>Affects Versions: 1.4.0
>Reporter: Venki Korukanti
>Assignee: Venki Korukanti
> Fix For: 1.5.0
>
>
> There have been few bug fixes in Hive SerDes since Hive 1.0.0. Its good to 
> update the Hive storage plugin to work with latest stable Hive version 
> (1.2.1), so that HiveRecordReader can use the latest SerDes.
> Compatibility when working with lower versions (v1.0.0 - currently supported 
> version) of Hive servers: There are no metastore API changes between Hive 
> 1.0.0 and Hive 1.2.1 that affect how Drill's Hive storage plugin is 
> interacting with Hive metastore. Tested to make sure it works fine. So users 
> can use Drill to query Hive 1.0.0 (currently supported) and Hive 1.2.1 (new 
> addition in this JIRA).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Resolved] (DRILL-3547) IndexOutOfBoundsException on directory with ~20 subdirectories

[Arina Ielchiieva (JIRA)]

 [ 
https://issues.apache.org/jira/browse/DRILL-3547?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Arina Ielchiieva resolved DRILL-3547.
-
Resolution: Cannot Reproduce

> IndexOutOfBoundsException on directory with ~20 subdirectories
> --
>
> Key: DRILL-3547
> URL: https://issues.apache.org/jira/browse/DRILL-3547
> Project: Apache Drill
>  Issue Type: Bug
>  Components: Functions - Drill
>Affects Versions: 1.1.0
> Environment: RHEL 7
>Reporter: Philip Deegan
>Assignee: Arina Ielchiieva
> Fix For: 1.6.0
>
>
> Tested on 1.1 with commit id:
> {noformat}
> 0: jdbc:drill:zk=local> select commit_id from sys.version;
> +---+
> | commit_id |
> +---+
> | e3fc7e97bfe712dc09d43a8a055a5135c96b7344  |
> +---+
> {noformat}
> Directory has child directories "a" to "u", each contain json files.
> Running the query on each subdirectory indivudually does not cause an error.
> {noformat}
> java.lang.RuntimeException: java.sql.SQLException: SYSTEM ERROR: 
> IndexOutOfBoundsException: index: 0, length: 1 (expected: range(0, 0))
> Fragment 1:2
> [Error Id: 69a0879f-f718-4930-ae6f-c526de05528c on 
> ip-172-31-29-60.eu-central-1.compute.internal:31010]
>   at sqlline.IncrementalRows.hasNext(IncrementalRows.java:73)
>   at 
> sqlline.TableOutputFormat$ResizingRowsProvider.next(TableOutputFormat.java:87)
>   at sqlline.TableOutputFormat.print(TableOutputFormat.java:118)
>   at sqlline.SqlLine.print(SqlLine.java:1583)
>   at sqlline.Commands.execute(Commands.java:852)
>   at sqlline.Commands.sql(Commands.java:751)
>   at sqlline.SqlLine.dispatch(SqlLine.java:738)
>   at sqlline.SqlLine.begin(SqlLine.java:612)
>   at sqlline.SqlLine.start(SqlLine.java:366)
>   at sqlline.SqlLine.main(SqlLine.java:259)
> {noformat}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)